Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Michal Kubecek <mkubecek@suse.cz>]

On Wed, May 06, 2020 at 12:08:35PM -0700, Cong Wang wrote:
> On Tue, May 5, 2020 at 10:26 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> >
> > On Tue, May 05, 2020 at 03:35:27PM -0700, Cong Wang wrote:
> > > On Tue, May 5, 2020 at 3:27 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > > > On Tue, May 05, 2020 at 02:58:19PM -0700, Cong Wang wrote:
> > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > index 522288177bbd..ece50ae346c3 100644
> > > > > --- a/net/core/dev.c
> > > > > +++ b/net/core/dev.c
> > > > > @@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
> > > > >                       netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> > > > >                                  &feature, lower->name);
> > > > >                       lower->wanted_features &= ~feature;
> > > > > -                     netdev_update_features(lower);
> > > > > +                     __netdev_update_features(lower);
> > > > >
> > > > >                       if (unlikely(lower->features & feature))
> > > > >                               netdev_WARN(upper, "failed to disable %pNF on %s!\n",
> > > >
> > > > Wouldn't this mean that when we disable LRO on a bond manually with
> > > > "ethtool -K", LRO will be also disabled on its slaves but no netlink
> > > > notification for them would be sent to userspace?
> > >
> > > What netlink notification are you talking about?
> >
> > There is ethtool notification sent by ethnl_netdev_event() and rtnetlink
> > notification sent by rtnetlink_event(). Both are triggered by
> > NETDEV_FEAT_CHANGE notifier so unless I missed something, when you
> > suppress the notifier, there will be no netlink notifications to
> > userspace.
> 
> Oh, interesting, why ethtool_notify() can't be called directly, for example,
> in netdev_update_features()? To me, using a NETDEV_FEAT_CHANGE
> event handler seems unnecessary for ethtool netlink.

It is certainly an option and as this is the only use of netdev
notifiers in ethtool code, it might even be more convenient. I rather
felt that when the call of notifier chain is in netdev_features_change()
already, it would be cleaner to use it. But my point rather was that the
NETDEV_FEAT_CHANGE event is used for more than only feature propagation
between upper/lower devices so that suppressing it may have unwanted
side effects (ethtool notifications were of course the first example
that came to my mind).

> BTW, as pointed out by Jay, actually we only need to skip
> NETDEV_FEAT_CHANGE for failure case, so I will update my patch.

Sounds like a good idea. I was wondering about this myself yesterday but
I didn't have time to look into it deeper so I only realized the problem
would probably be a difference between features and wanted_features.

Michal