netdev - net: netdev_sync_lower

lists.openwall.net		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 linux-cve-announce 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 <mirq-linux@...e.qmqm.pl>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        Jarod Wilson <jarod@...hat.com>
Cc:     netdev@...r.kernel.org
Subject: net: netdev_sync_lower_features()

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