Delayed context for SIOCADDMULTI

[glebius]

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.

[• hselasky]

Please add code to drain this new task.

[glebius]

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

[• hselasky]

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]

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]

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.

[glebius]

Remove refcounting and rely on taskqueue_drain().

[glebius]

sys/net/if.c

3544 ↗

(On Diff #63671)

THREAD_CAN_SLEEP should include a check for epochnest?

I'm discussing that with Kostik.

[• hselasky]

Looks good to me.

[dheerajk_netapp.com]

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.

[dheerajk_netapp.com]

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.

[• hselasky]

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.

[• hselasky]

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

[dheerajk_netapp.com]

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.

[dheerajk_netapp.com]

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.

[glebius]

@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