Re: [PATCH] c/r: Add AF_UNIX support (v3)

[Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>]

Dan Smith wrote:
> OL> (You'll need to slightly refactor sock_setsockopt() for that).
> 
> It seems much easier and less invasive to just check against
> sysctl_{r,w}mem_max.  I've got that added to the patch now.

But there are two cases:  if you are CAP_NET_ADMIN you are allowed
to go beyond that limit. So you need to add that test too.

And in general, this helps to keep the checks - be it security,
resource limits, or whatever - in one place, instead of having
to duplicate code and, more importantly, risk getting out of
sync with the original checks (e.g., if sock_setsockopt changes).

> 
> OL> 2) 	s = socket(.., SOCK_DGRAM,...);
> OL> 	bind(s, any_addr);
> OL> 	connect(s, other_addr, ...);
> --> now s is connected, but after restart you can't connect another
> OL> socket to it because the address wasn't bind() properly.
> 
> Okay, I guess that's true.  So, since there isn't a "I'm bound, but
> not listening or connected" flag anywhere, does it suffice to bind()
> any socket that is not connected but that does have a local address?
> Sockets that get a local address via connect() should never transition
> through that state, so I think that should work.  At least for INET,
> any socket that is restored into a connected state is properly hashed
> such that another socket can't bind() to its local address (tested).

Yes, with the exception below (for UNIX)...

> 
> OL> (And if the address was a pathname, but already unlinked, then
> OL> also unlink after the bind, FWIW).
> 
> For path-based UNIX sockets, we don't care about this exclusion,
> right?  As long as we make the socket owner think everything is as it

You are correct in that you don't need it for the scenario you
presented below.

But we do care, because it is necessary to do the unlink() after the
bind(), like you do for listening sockets, for this scenario:

	s = socket()
	bind(s, pathname)
	unlink(pathname)
				<---- checkpoint/restart
	r = socket()
	bind(r, pathname)

The second bind() will succeed on the original system, but will
fail on the restarted system, unless you do that.

> was, that is.  Given that a normal system doesn't fail the bind of b
> in this case:
> 
>   a = socket(AF_UNIX);
>   b = socket(AF_UNIX);
> 
>   bind(a, addr);
>   unlink(addr);
>   bind(b, addr);
> 

BTW, I just looked again at the code, and I'm concerned about:

+		if (!un->linked) {
+			struct sockaddr_un *sun =
+				(struct sockaddr_un *)&h->laddr;
+			ret = sock_unix_unlink(sun->sun_path);
+		}

You need to verify that the address is not abstract, because
I'm not sure what sock_unix_unlink() would do given the empty
string. Usually this is filtered higher in the VFS (getname).

Oren.