Page MenuHomeFreeBSD

Rework linux accept(2)
ClosedPublic

Authored by trasz on Jun 26 2020, 8:20 AM.

Details

Summary

Rework linux accept(2). This makes the code flow easier to follow,
and fixes a bug where calling accept(2) could result in closing fd 0.

Note that the code still contains a number of problems: it makes
assumptions about l_sockaddr_in being the same as sockaddr_in,
the EFAULT-related code looks like it doesn't work at all, and the
socket type check is racy. Those will be addressed later on;
I'm trying to work in small steps to avoid breaking one thing while
fixing another.

It fixes Redis, among other things.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32019
Build 29552: arc lint + arc unit

Event Timeline

trasz requested review of this revision.Jun 26 2020, 8:20 AM
sys/compat/linux/linux_socket.c
646

Is EFAULT possible from kern_accept4() ?

651

So this is racy.

661

This default label does not serve any use.

680

There is one case where kern_accept4() can return error without assigning NULL to *fp.

trasz added inline comments.
sys/compat/linux/linux_socket.c
646

I'm not sure. Perhaps not, but I'd rather not mix unrelated changes together.

651

I guess. But it's no worse than before, right?

I'm not sure what's the best way of dealing with this problem. We could just add ABI checks at places where the errno is being returned, ie somewhere in kern_accept4(), but that wouldn't work with linux(4) being a loadable module.

sys/compat/linux/linux_socket.c
646

IMO it is directly related.

BTW does the code assumes that l_sockaddr_in is same as sockaddr_in ?

651

I think you need a different interface for kern_accept4(). It should take struct file * instead of file descriptor, and return struct file * as well. This would also allow to remove the uglyness with td_retval[0] below.

sys/compat/linux/linux_socket.c
646

How is it related? (The reason why I'm trying to avoid unrelated, or tangentially related changes is that it makes it harder to debug later on.)

As for the assumption - it does. I'll get back to it later on; I need to review all the l_whatever types anyway.

651

I'd rather not do that now. Yes, the check is buggy, but it might turn out it's not needed in the first place - it looks like something that was added to fix a synthetic LTP test and not a real bug.

sys/compat/linux/linux_socket.c
646

Indeed, I have no idea why you claim that some bugs are unrelated. With the subject 'rework linux_accept(2)' I just point out bugs that I see remaining in the code.

sys/compat/linux/linux_socket.c
646

Ah, ok. So, by "unrelated" here I mean it's not made worse or better by the current patch. I should probably point out the remaining issues in the commit log.

This revision is now accepted and ready to land.Jul 1 2020, 12:10 AM