Date: Wed, 18 Jun 2008 16:19:06 +0200 From: Marko Zec <zec@icir.org> To: freebsd-virtualization@freebsd.org Subject: Re: V_* meta-symbols and locking Message-ID: <200806181619.07026.zec@icir.org> In-Reply-To: <48588595.7020709@gritton.org> References: <48588595.7020709@gritton.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 18 June 2008 05:48:37 James Gritton wrote:
> Like everything I have to say about the V_* issue, perhaps this
> doesn't apply to the vnet stuff. But to the two symbols I currently
> care about, hostname and rootvnode, locking is a problem.
You are most probably right about the current code not sufficiently
protecting the "hostname" global from concurrent access, but I don't
see how V_ macroization / virtualization adds or changes anything to
this particular issue. The same goes for virtualizing the networking
state - if there is a locking issue in a newtorking subsystem,
virtualization should not make such issues any more or less pronounced.
That doesn't mean we shouldn't be looking into solving such issues, but
I'd prefer to decouple such efforts from the initial push for
virtualizing the networking stack, which I think should keep the
changes to existing subsystem's program logic / flow to the absolute
minimum.
Marko
> Current kernel code plays fast and loose with both these symbols.
> Check out getcredhostname for example:
>
> void
> getcredhostname(struct ucred *cred, char *buf, size_t size)
> {
> struct prison *pr;
>
> pr = cred->cr_prison;
> if (pr != &prison0) {
> mtx_lock(&pr->pr_mtx);
> strlcpy(buf, (pr->pr_flags & PR_NOHOST)
> ? hostname : pr->pr_host, size);
> mtx_unlock(&pr->pr_mtx);
> } else
> strlcpy(buf, hostname, size);
> }
>
> In the prison case, it nicely locks the prison record. But for the
> global hostname, it just copies it. The hostname sysctl is no better
> about setting it. And rootvnode is referred to all over the place
> without any sort of lock - pretty safe since it's not expected to
> change (though it theoretically can).
>
> This same no-locking assumption seems to be going on with V_hostname.
> But now this macro applies not only to the "real" hostname but to the
> "virtual" one as well - no locking the vimage record. As I try to
> add a similar macro to my new jail framework, I find I can't.
> Instead of a mere variable redirection, I need to lock-copy-unlock
> much like getcredhostname does. Luckily, much hostname access is
> already jail-aware. But anything using the "real" hostname should
> have the same locking on prison0. Perhaps not wholly necessary since
> it's just a string that we know will always have a null byte at the
> end of the buffer, but still good form and unknown prevention. And
> in the case of actually virtual hostnames, it's essential since
> they'll be changing from fixed arrays in struct prison into pointers
> that may be freed.
>
> Rootvnode is a stickier problem. There's much more code that refers
> to it, and it's a more essential part of the system. I don't relish
> digging in everywhere and changing the whole rootvnode paradigm with
> locking. So instead my solution is to make the jail "path" parameter
> (and thus root vnode) set-once. So as long as the V_rootvnode is
> taken from a context that will remain for the duration of its use
> (curthread is a good bet), it will be safe to access it without
> locks. In particular, the real rootvnode that lives at prison0 isn't
> going anywhere.
>
> So in summary:
>
> I won't use V_hostname (or G_hostname), opting for explicit locking.
>
> I will V_rootvnode (and perhaps G_rootvnode).
>
> All the other network-related V_stuff may deserve a look, but it out
> of my purview.
>
> - Jamie
> _______________________________________________
> freebsd-virtualization@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
> To unsubscribe, send any mail to
> "freebsd-virtualization-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200806181619.07026.zec>