On Mon, May 18, 2020 at 1:32 AM Michał Mirosław <mirq-li...@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.
