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 ?

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

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.

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

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

	*(struct sockaddr_in *)sa = *sin;

Why the read lock get dropped ?

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

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.

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

Why do we need this?

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.

Thanks!

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.

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.