lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 18 May 2020 10:32:39 +0200
From:   Michał Mirosław <>
To:     Cong Wang <>,
        Jarod Wilson <>
Subject: net: netdev_sync_lower_features()


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;

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

Powered by blists - more mailing lists