Re: [PATCH net v4 01/12] net: core: limit nested device depth

[Taehee Yoo <ap420073@gmail.com>]

On Sun, 29 Sep 2019 at 04:36, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi,
>
> >  int netdev_walk_all_upper_dev_rcu(struct net_device *dev,
> >                                 int (*fn)(struct net_device *dev,
> >                                           void *data),
> >                                 void *data)
> >  {
> [...]
> >       }
> >
> >       return 0;
> > +
> >  }
>
> that seems like an oversight, probably from editing the patch in
> different versions?
>

I will fix this in a v5 patch.

> > +static int __netdev_update_upper_level(struct net_device *dev, void *data)
> > +{
> > +     dev->upper_level = __netdev_upper_depth(dev) + 1;
> > +     return 0;
> > +}
> > +
> > +static int __netdev_update_lower_level(struct net_device *dev, void *data)
> > +{
> > +     dev->lower_level = __netdev_lower_depth(dev) + 1;
> > +     return 0;
> > +}
>
> Is there any point in the return value here? You don't really use it,
> afaict? I guess I might see the point if it was used for tail-call
> optimisation or such?
>

These functions are used as a callback function of
netdev_walk_all_{upper/lower}_dev(). So these return types are needed.

>
> Also, I dunno, I guess netdevs aren't as much under pressure as SKBs :-)
> but do we actually gain much from storing the nesting level at all? You
> have to maintain it all the time anyway when adding/removing and that's
> the only place where you also check it, so perhaps it wouldn't be that
> bad to just count at that time?
>
> But then again the counting would probably be recursive again ...
>
> >       return 0;
> > +
> >  }
> >  EXPORT_SYMBOL_GPL(netdev_walk_all_lower_dev_rcu);
>
> same nit as above
>

I will fix this in a v5 patch too.

> > +     __netdev_update_upper_level(dev, NULL);
> > +     netdev_walk_all_lower_dev(dev, __netdev_update_upper_level, NULL);
> > +
> > +     __netdev_update_lower_level(upper_dev, NULL);
> > +     netdev_walk_all_upper_dev(upper_dev, __netdev_update_lower_level, NULL);
>
> Actually, if I'm reading this correctly you already walk all the levels
> anyway? Then couldn't you calculate the depth at this time and return
> it, instead of storing it? Though, if it actually overflowed then you'd
> have to walk *again* to undo that?
>
> Hmm, actually, if you don't store the value you don't even need to walk
> here I guess, or at least you would only have to do it to verify you
> *can* attach, but wouldn't have to in detach?
>
> So it looks to me like on attach (i.e. this code, quoted from
> __netdev_upper_dev_link) you're already walking the entire graph to
> update the level values, and could probably instead calculate the
> nesting depth to validate it?
> And then on netdev_upper_dev_unlink() you wouldn't even have to walk the
> graph at all, since you only need that to update the values that you
> stored.
>
> But maybe I'm misinterpreting this completely?
>

Without storing level storing, a walking graph routine is needed only
once. The routine would work as a nesting depth validator.
So that the detach routine doesn't need to walk the graph.
Whereas, in this patch, both attach and detach routine need to
walk graph. So, storing nesting variable way is slower than without
storing nesting variable way because of the detach routine's updating
upper and lower level routine.

But I'm sure that storing nesting variables is useful because other
modules already using nesting level values.
Please look at vlan_get_encap_level() and usecases.
If we don't provide nesting level variables, they should calculate
every time when they need it and this way is easier way to get a
nesting level. There are use-cases of lower_level variable
in the 11th patch.

Thank you

> Thanks,
> johannes
>
>