sockets: don't malloc/free sockaddr memory on getpeername/getsockname

[glebius]

Summary

Just like it was done for accept(2) in XXXXXXXXX, use same approach
for two simplier syscalls that return socket addresses. Although,
these two syscalls aren't performance critical, this change generalizes
some code between 3 syscalls trimming code size.

Following example of accept(2), provide VNET-aware and INVARIANT-checking
wrappers sopeeraddr() and sosockaddr() around protosw methods.

[dchagin]

just out of curiosity, why to not to add new kernel interfaces aka
kern_getsockname3(struct thread *td, int fd, struct sockaddr_storage *ss)
and below, into the pr methods?

[glebius]

In D42694#974081, @dchagin wrote:

just out of curiosity, why to not to add new kernel interfaces aka
kern_getsockname3(struct thread *td, int fd, struct sockaddr_storage *ss)
and below, into the pr methods?

Read your comment several times and can't answer anything better than "why to add such a method?" What is the benefit the presence of the method would bring?

[zlei]

sys/compat/linux/linux_socket.c

1125

len = ss.ss_len;

I'm not understanding this assignment.

The original implementation copyout the MIN(len, sa.sa_len), but with this change ss.ss_len is copyout.

So is the original implementation buggy , or this behavior change is intended ?

1138

The len is copyin from userspace. I guess the sanity check should be preserved.

sys/netgraph/ng_socket.c

514–517

Maybe we need to move the assignment into the following block ?

	if (pcbp->sockdata->node != NULL) {

	}

So that in case EINVAL the struct sockaddr *sa will keep untouched.

sys/netinet/sctp_usrreq.c

7372

I'm wondering if we need on stack temporary var.

	struct sockaddr_in tmp;
	struct sockaddr_in *sin = &tmp;
....

	*(struct sockaddr_in *)sa = *sin;

sys/netinet6/in6_pcb.c

530

Why the read lock get dropped ?

sys/ofed/drivers/infiniband/ulp/sdp/sdp_main.c

194

With this change and D42635 , sdp_sockaddr() is no longer needed.

[glebius]

sys/compat/linux/linux_socket.c

1125

This is intentional. In the previous accept() patch I noted that now we start reporting the actual sockaddr len:

While rewriting accept(2) make 'addrlen' a true in/out parameter, reporting
required length in case if provided length was insufficient. Our manual
page accept(2) and POSIX don't explicitly require that, but one can read
the text as they do. Linux also does that. Update tests accordingly.

And added a test case for that. This change did the same for getsockname()/getpeername(). Probably for Linux emulation this is even more important than for native syscalls.

1138

This check isn't needed. The EINVAL will come from linux_copyout_sockaddr(). Removing this line makes getpeername() be same as getsockname().

sys/netinet/sctp_usrreq.c

7372

Why do we need this?

sys/netinet6/in6_pcb.c

530

These fields of inpcb should be stable in the scope of the syscall. Packets from network can't modify them.

Well, if you write a brain dead program that would run multiple threads doing bind(2)/connect(2) against getsockaddr(2), they would race. But they can also race just in the user context with all the locking present in kernel. The program won't give you consistent results, as sometimes getsockaddr() will run before connect(2) and sometimes after (in the userland). The only difference with such brain dead program would be that now theoretically getsockname() can return an IP address from old connection and port from new one (or vice versa). Do we care about such braindead test or not? My call is we don't.

sys/ofed/drivers/infiniband/ulp/sdp/sdp_main.c

194

Thanks!

[zlei]

sys/netinet/sctp_usrreq.c

7372

Why do we need this?

The reason is same with previous comment:

So that in case EINVAL the struct sockaddr *sa will keep untouched

Well in general I personally would prefer not to touch input variables incase ERRORs. I think that is good practice. Also the logic is quite clear and readers will not have to waste time thinking or evaluating why the function errors but my variable get touched, and is that a valid combination ? Will that be harmful in some cases ?.

Anyway the caller should always check return value so touch input variables incase ERROR seems to be good.

[glebius]

sys/netinet/sctp_usrreq.c

7372

I agree with suggested principle in general, but IMHO it shouldn't be brought down to scope of every function. This particular function expects to be provided with stack allocated sockaddr pointer that is fine to write to. If the function returns an error, the syscall implementation will not copyout(9) the sockaddr on stack to the userland.