Re: [PATCH net v2 2/3] net: core: add netdev_class_has_file_ns() helper function

[Greg KH]

On Sun, Apr 05, 2020 at 02:18:22AM +0900, Taehee Yoo wrote:
> On Sun, 5 Apr 2020 at 00:51, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> 
> Hi Greg,
> Thank you for your review!
> 
> > On Sat, Apr 04, 2020 at 02:19:09PM +0000, Taehee Yoo wrote:
> > > This helper function is to check whether the class file "/sys/class/net/*"
> > > is existing or not.
> > > In the next patch, this helper function will be used.
> > >
> > > Reported-by: syzbot+830c6dbfc71edc4f0b8f@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Fixes: b76cdba9cdb2 ("[PATCH] bonding: add sysfs functionality to bonding (large)")
> > > Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> > > ---
> > >
> > > v1 -> v2:
> > >  - use class_has_file_ns(), which is introduced by the first patch.
> > >
> > >  include/linux/netdevice.h | 2 +-
> > >  net/core/net-sysfs.c      | 6 ++++++
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 130a668049ab..a04c487c0975 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -4555,7 +4555,7 @@ int netdev_class_create_file_ns(const struct class_attribute *class_attr,
> > >                               const void *ns);
> > >  void netdev_class_remove_file_ns(const struct class_attribute *class_attr,
> > >                                const void *ns);
> > > -
> > > +bool netdev_class_has_file_ns(const char *name, const void *ns);
> > >  static inline int netdev_class_create_file(const struct class_attribute *class_attr)
> > >  {
> > >       return netdev_class_create_file_ns(class_attr, NULL);
> > > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> > > index cf0215734ceb..8a20d658eff0 100644
> > > --- a/net/core/net-sysfs.c
> > > +++ b/net/core/net-sysfs.c
> > > @@ -1914,6 +1914,12 @@ void netdev_class_remove_file_ns(const struct class_attribute *class_attr,
> > >  }
> > >  EXPORT_SYMBOL(netdev_class_remove_file_ns);
> > >
> > > +bool netdev_class_has_file_ns(const char *name, const void *ns)
> > > +{
> > > +     return class_has_file_ns(&net_class, name, ns);
> > > +}
> > > +EXPORT_SYMBOL(netdev_class_has_file_ns);
> >
> > Again, this feels broken, it can not solve a race condition.
> >
> 
> This function is considered to be used under rtnl mutex and
> I assume that no one could use "/sys/class/net/*" outside of rtnl mutex.
> So, I think it returns the correct information under rtnl mutex.

But you are creating a globally exported function that can be called
from anywhere, and as such, is not useful because it has no locking or
hints of how to use it correctly at all.

Again, don't push this "solution" down to sysfs to solve, you know if
you have a device that is not cleaned up yet, so don't try to
rename/create a device of the same name before that is finished.

thanks,

greg k-h