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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

trasz requested review of this revision.Jun 26 2020, 8:20 AM
sys/compat/linux/linux_socket.c
646 ↗(On Diff #73687)

Is EFAULT possible from kern_accept4() ?

651 ↗(On Diff #73687)

So this is racy.

661 ↗(On Diff #73687)

This default label does not serve any use.

682 ↗(On Diff #73687)

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 ↗(On Diff #73687)

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

651 ↗(On Diff #73687)

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 ↗(On Diff #73687)

IMO it is directly related.

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

651 ↗(On Diff #73687)

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 ↗(On Diff #73687)

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 ↗(On Diff #73687)

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 ↗(On Diff #73687)

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 ↗(On Diff #73687)

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