| Message ID | 20200404141909.26399-1-ap420073@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: core: avoid unexpected situation in namespace change routine | expand |
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@syzkaller.appspotmail.com > Fixes: b76cdba9cdb2 ("[PATCH] bonding: add sysfs functionality to bonding (large)") > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > 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. greg k-h
On Sun, 5 Apr 2020 at 00:51, Greg KH <gregkh@linuxfoundation.org> 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@syzkaller.appspotmail.com > > Fixes: b76cdba9cdb2 ("[PATCH] bonding: add sysfs functionality to bonding (large)") > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > 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. Thanks a lot! Taehee Yoo
On Sun, Apr 05, 2020 at 02:18:22AM +0900, Taehee Yoo wrote: > On Sun, 5 Apr 2020 at 00:51, Greg KH <gregkh@linuxfoundation.org> 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@syzkaller.appspotmail.com > > > Fixes: b76cdba9cdb2 ("[PATCH] bonding: add sysfs functionality to bonding (large)") > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > --- > > > > > > 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
On Sun, 5 Apr 2020 at 16:32, Greg KH <gregkh@linuxfoundation.org> wrote: > Hi Greg, Thank you for the review! > On Sun, Apr 05, 2020 at 02:18:22AM +0900, Taehee Yoo wrote: > > On Sun, 5 Apr 2020 at 00:51, Greg KH <gregkh@linuxfoundation.org> 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@syzkaller.appspotmail.com > > > > Fixes: b76cdba9cdb2 ("[PATCH] bonding: add sysfs functionality to bonding (large)") > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > > --- > > > > > > > > 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. > Yes, I agree with that. > 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. > Okay, Thank you for that. I will find another way to fix it. Thanks a lot! Taehee Yoo
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); + int __init netdev_kobject_init(void) { kobj_ns_type_register(&net_ns_type_operations);
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@syzkaller.appspotmail.com Fixes: b76cdba9cdb2 ("[PATCH] bonding: add sysfs functionality to bonding (large)") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- 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(-)