net: netdev_sync_lower_features()

net: netdev_sync_lower_features()

[Michał Mirosław]

Hi!

I just saw commit dd912306ff008 ("net: fix a potential recursive
NETDEV_FEAT_CHANGE") landing in Linux master. The problem with it (or
rather: with the netdev_sync_lower_features() function) is that
netdev_update_features() is allowed to change more than one feature
at a time, including force-enabling other feature than one that is
being disabled. I think that a better fix would be to trigger
notification only after all features are updated (outside of the loop).
When you consider net effect of the function, the loop's added value
is only to print debug messages. Other than that it's equivalent to:

lower->wanted_features &= ~upper_disables;
netdev_update_features(lower);

The problem of spurious notification can be fixed in
__netdev_update_features() by saving dev->features at the start and
only return -1 when it really changed.

BTW, I don't remember seeing the original commit fd867d51f889 ("net/core:
generic support for disabling netdev features down stack"). It looks
like it could have be implemented just by recursively calling
netdev_update_features() for lower devices - netdev_sync_upper_features()
called on their behalf should do just what netdev_sync_lower_features()
is doing now in the context of upper device, and also respect all the
other constraints we have on features.

Best Regards,
Michał Mirosław

Re: net: netdev_sync_lower_features()

[Cong Wang]

On Mon, May 18, 2020 at 1:32 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> Hi!
>
> I just saw commit dd912306ff008 ("net: fix a potential recursive
> NETDEV_FEAT_CHANGE") landing in Linux master. The problem with it (or
> rather: with the netdev_sync_lower_features() function) is that
> netdev_update_features() is allowed to change more than one feature
> at a time, including force-enabling other feature than one that is
> being disabled. I think that a better fix would be to trigger
> notification only after all features are updated (outside of the loop).

Sounds reasonable. But I don't think this problem is introduced by
my commit, my commit merely skips the notification for the
failure.

> When you consider net effect of the function, the loop's added value
> is only to print debug messages. Other than that it's equivalent to:
>
> lower->wanted_features &= ~upper_disables;
> netdev_update_features(lower);
>
> The problem of spurious notification can be fixed in
> __netdev_update_features() by saving dev->features at the start and
> only return -1 when it really changed.

Please send a patch.

Thanks.