Delayed context for SIOCADDMULTI
ClosedPublic
Actions

Authored by glebius on Oct 25 2019, 5:13 PM.

Details

Reviewers

• hselasky
erj

Group Reviewers

network

Commits

rS354149: There is a long standing problem with multicast programming for NICs

Summary

There is a long standing problem that IPv6 may call into if_ioctl(SIOCADDMULTI)
within the network input path, and some drivers may sleep in their if_ioctl.
This was always a problem with e1000 family of drivers, but conversion to iflib
has hidden the problem moving e1000 input packet processing into separate
taskqueue. The assertion no longer triggered, but the problem remained. Now
sleep won't block an interrupt, but will block the taskqueue thread, thus will
block the driver.

Now that all input network packet processing moved into network epoch context
we got assertion triggered again - sleep is prohibited.

Since nobody checks for return of if_ioctl() and there is nobody to report
the result to, there is no reason to run it synchronously. Thus, create
a task for that and in non-syscall contexts if_addmulti() should run the
if_ioctl via task.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

glebius created this revision.Oct 25 2019, 5:13 PM

Herald added subscribers: ae, imp. · View Herald TranscriptOct 25 2019, 5:13 PM

Harbormaster completed remote builds in B27222: Diff 63669.Oct 25 2019, 5:13 PM

Please add code to drain this new task.

Drain the task. I'm not sure this is required as we reference the ifp, but
better safe than sorry.

Harbormaster completed remote builds in B27224: Diff 63671.Oct 25 2019, 6:40 PM

• hselasky added inline comments.Oct 25 2019, 8:26 PM

sys/net/if.c
3544 ↗	(On Diff #63671)	THREAD_CAN_SLEEP should include a check for epochnest?
3577 ↗	(On Diff #63671)	You should release the "ifp" pending number of times?

• hselasky added inline comments.Oct 25 2019, 8:27 PM

sys/net/if.c
3547 ↗	(On Diff #63671)	Maybe drop the refcounting. Makes it more complicated when calls are coalesched, and depend on drain of the taskqueue instead.

• hselasky added inline comments.Oct 25 2019, 8:30 PM

sys/net/if.c
1136 ↗	(On Diff #63671)	Can you check if this waits for any pending tasks? Or just cancel them w/o callback? Then you need to clean up the refcounting. As suggested, maybe the best is just to skip that.

Remove refcounting and rely on taskqueue_drain().

Harbormaster completed remote builds in B27227: Diff 63679.Oct 25 2019, 9:27 PM

glebius marked an inline comment as done.Oct 25 2019, 9:28 PM

glebius added inline comments.

sys/net/if.c
3544 ↗	(On Diff #63671)	THREAD_CAN_SLEEP should include a check for epochnest? I'm discussing that with Kostik.

Looks good to me.

This revision is now accepted and ready to land.Oct 25 2019, 9:59 PM

Closed by commit rS354149: There is a long standing problem with multicast programming for NICs (authored by glebius). · Explain WhyOct 29 2019, 5:36 PM

This revision was automatically updated to reflect the committed changes.

glebius added a commit: rS354149: There is a long standing problem with multicast programming for NICs.

Shouldn't there be a similar fix for if_delmulti? This change only addresses if_addmulti. If the change is not required for if_delmulti, can you add the justification for why it is not required.

Herald added a subscriber: melifaro. · View Herald TranscriptApr 6 2022, 2:26 PM

Shouldn't the THREAD_CAN_SLEEP also check for in_epoch() test? I have come across threads that have the td_no_sleeping set to 0 and are inside an epoch section.

I think a delmultitask would be appropriate to avoid races on the if_ioctl handler, adding and removing multicast addresses.

Regarding in_epoch() in non-sleepable context, this is not an issue. Entering and exiting epoch should not block or sleep.

If you look at:

void
_epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et EPOCH_FILE_LINE)
{
...
        THREAD_NO_SLEEPING();

You find that in_epoch() implies THREAD_CAN_SLEEP() == 0 .

--HPS

In D22154#788671, @hselasky wrote:

I think a delmultitask would be appropriate to avoid races on the if_ioctl handler, adding and removing multicast addresses.

Regarding in_epoch() in non-sleepable context, this is not an issue. Entering and exiting epoch should not block or sleep.

Is there a change that adds if_delmultitask similar to r354149 which added if_addmultitask? I don't see one.

In D22154#788672, @hselasky wrote:
If you look at:
void
_epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et EPOCH_FILE_LINE)
{
...
        THREAD_NO_SLEEPING();
You find that in_epoch() implies THREAD_CAN_SLEEP() == 0 .

--HPS

Thanks for the responses.

@dheerajk_netapp.com A more generic approach is needed here. See also: https://lists.freebsd.org/archives/dev-commits-src-main/2022-April/005560.html