Page MenuHomeFreeBSD
Differential D25461

Rework linux accept(2)
ClosedPublic
Actions

Authored by trasz on Jun 26 2020, 8:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 21, 7:04 PM
Unknown Object (File)
Wed, Apr 16, 10:20 PM
Unknown Object (File)
Wed, Apr 16, 8:16 PM
Unknown Object (File)
Sun, Apr 13, 7:17 PM
Unknown Object (File)
Tue, Apr 8, 1:59 AM
Unknown Object (File)
Mar 7 2025, 8:31 PM
Unknown Object (File)
Mar 5 2025, 2:04 AM
Unknown Object (File)
Mar 4 2025, 6:22 PM
View All 63 Files
Subscribers
imp

Details

Reviewers
kib
Group Reviewers
Linux Emulation
Commits
rS362833: Rework linux accept(2). This makes the code flow easier to follow,
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 Passed
Unit
No Test Coverage
Build Status
Buildable 32019
Build 29552: arc lint + arc unit

Event Timeline

trasz created this revision.Jun 26 2020, 8:20 AM
Herald added a subscriber: imp. · View Herald TranscriptJun 26 2020, 8:20 AM
Harbormaster completed remote builds in B31970: Diff 73687.Jun 26 2020, 8:20 AM
trasz requested review of this revision.Jun 26 2020, 8:20 AM
trasz added reviewers: Linux Emulation, kib.Jun 26 2020, 8:21 AM
kib added inline comments.Jun 26 2020, 6:17 PM
sys/compat/linux/linux_socket.c
647

Is EFAULT possible from kern_accept4() ?

652

So this is racy.

662

This default label does not serve any use.

681

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

trasz updated this revision to Diff 73794.Jun 28 2020, 12:57 AM

Fix issues.

Harbormaster completed remote builds in B32019: Diff 73794.Jun 28 2020, 12:57 AM
trasz marked 2 inline comments as done.Jun 28 2020, 1:00 AM
trasz added inline comments.
sys/compat/linux/linux_socket.c
647

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

652

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.

kib added inline comments.Jun 28 2020, 4:25 AM
sys/compat/linux/linux_socket.c
647

IMO it is directly related.

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

652

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.

trasz added inline comments.Jun 29 2020, 7:01 PM
sys/compat/linux/linux_socket.c
647

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.

652

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.

kib added inline comments.Jun 30 2020, 6:49 AM
sys/compat/linux/linux_socket.c
647

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.

trasz added inline comments.Jun 30 2020, 3:47 PM
sys/compat/linux/linux_socket.c
647

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.

trasz edited the summary of this revision. (Show Details)Jun 30 2020, 3:49 PM
kib accepted this revision.Jul 1 2020, 12:10 AM
This revision is now accepted and ready to land.Jul 1 2020, 12:10 AM
Closed by commit rS362833: Rework linux accept(2). This makes the code flow easier to follow, (authored by trasz). · Explain WhyJul 1 2020, 10:37 AM
This revision was automatically updated to reflect the committed changes.
trasz added a commit: rS362833: Rework linux accept(2). This makes the code flow easier to follow,.

Revision Contents
Changeset List

PathSize
sys/
compat/
linux/
53 lines

Diff 73794

View Options

sys/compat/linux/linux_socket.c

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines