From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org --- drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c index 049ccc070ce56..cb5718d2669ee 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -862,6 +862,8 @@ static void cleanup_glue_dir(struct device *dev, struct kobject *glue_dir) return;
mutex_lock(&gdp_mutex); + if (!kobject_has_children(glue_dir)) + kobject_del(glue_dir); kobject_put(glue_dir); mutex_unlock(&gdp_mutex); } diff --git a/include/linux/kobject.h b/include/linux/kobject.h index e6284591599ec..5957c6a3fd7f9 100644 --- a/include/linux/kobject.h +++ b/include/linux/kobject.h @@ -113,6 +113,23 @@ extern void kobject_put(struct kobject *kobj); extern const void *kobject_namespace(struct kobject *kobj); extern char *kobject_get_path(struct kobject *kobj, gfp_t flag);
+/** + * kobject_has_children - Returns whether a kobject has children. + * @kobj: the object to test + * + * This will return whether a kobject has other kobjects as children. + * + * It does NOT account for the presence of attribute files, only sub + * directories. It also assumes there is no concurrent addition or + * removal of such children, and thus relies on external locking. + */ +static inline bool kobject_has_children(struct kobject *kobj) +{ + WARN_ON_ONCE(atomic_read(&kobj->kref.refcount) == 0); + + return kobj->sd && kobj->sd->dir.subdirs; +} + struct kobj_type { void (*release)(struct kobject *kobj); const struct sysfs_ops *sysfs_ops;
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
Queued for 4.4, thank you.
-- Thanks, Sasha
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
Wait, why is this needed?
And why only for 4.4? What about 4.9 and 4.14? Do you want to upgrade and suddenly hit the same "bug" that you fixed before?
There was a reason that I did not backport this to the stable tree when it was submitted, and that was because this was an odd race to ever hit. Are you hitting this in the real world without kobject deferred release enabled? And if so, are you hitting the WARN_ON that is added here?
I would like to hold off on this until some more information is provided, and at the least, if it is accepted, we add it to all of the stable trees, not just 4.4.y. So I'm going to go drop it from the 4.4.y queue for now.
thanks,
greg k-h
On Mon, Jan 28, 2019 at 10:58 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
Wait, why is this needed?
We have syzcaller/syzbot hits, and there is a reproducer. Is this sufficient ?
And why only for 4.4? What about 4.9 and 4.14? Do you want to upgrade and suddenly hit the same "bug" that you fixed before?
Good point. Sorry, that got lost. We are, after all, oly human. I'd ask Zubin to provide backports, or do it myself, but we'll have to resolve the issue you bring up below first.
There was a reason that I did not backport this to the stable tree when it was submitted, and that was because this was an odd race to ever hit. Are you hitting this in the real world without kobject deferred release enabled? And if so, are you hitting the WARN_ON that is added here?
I think we may need updated rules for stable. Many bug fixes are backported to stable releases without having been seen in the "real world" (whatever that means). At the same time we do see many races in the real world, many of them not fully understood. Our policy so far is to fix as many problems as possible if they are understood, in the hope that it fixes at least some of those problems seen in the field.
If there is a new rule that problems have to have been observed in the real world (ie without debugging options enabled, and without specific reproducer) before a patch is applied to stable, can we have that as generic rule that is not only selectively applied ?
We'll definitely continue fixing race conditions (and security issues, for that matter) in our branches. So far our policy was to request backport to -stable releases to limit deviation from stable releases as much as possible, to reduce our maintenance burden going forward, and to have others benefit from our work. We'll definitely continue doing that for ChromeOS branches. I would find it regrettable to have to revise our policy and bypass -stable, but we'll do that given no choice. We will not spend additional effort trying to prove if a problem is seen "in the real world"; that would be futile and a waste of time. If that is the new criteria, we'll have to keep our patches local.
Thanks, Guenter
On Tue, Jan 29, 2019 at 06:35:48AM -0800, Guenter Roeck wrote:
On Mon, Jan 28, 2019 at 10:58 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
Wait, why is this needed?
We have syzcaller/syzbot hits, and there is a reproducer. Is this sufficient ?
Nice, where?
And why only for 4.4? What about 4.9 and 4.14? Do you want to upgrade and suddenly hit the same "bug" that you fixed before?
Good point. Sorry, that got lost. We are, after all, oly human. I'd ask Zubin to provide backports, or do it myself, but we'll have to resolve the issue you bring up below first.
There was a reason that I did not backport this to the stable tree when it was submitted, and that was because this was an odd race to ever hit. Are you hitting this in the real world without kobject deferred release enabled? And if so, are you hitting the WARN_ON that is added here?
I think we may need updated rules for stable. Many bug fixes are backported to stable releases without having been seen in the "real world" (whatever that means). At the same time we do see many races in the real world, many of them not fully understood. Our policy so far is to fix as many problems as possible if they are understood, in the hope that it fixes at least some of those problems seen in the field.
I don't have a problem with backporting this, but it went in as a "fixes a theoritical issue, and let's WARN_ON if it really ever is hit".
So I would like to see where we are hitting that WARN_ON, as it sounds like you found an easy-to-reproduce way to do it.
If there is a new rule that problems have to have been observed in the real world (ie without debugging options enabled, and without specific reproducer) before a patch is applied to stable, can we have that as generic rule that is not only selectively applied ?
It's not a specific rule, it's just that this specific patch went through a 30+ email chain of us arguing about it, so I really want to see why this is needed, and why Ben was right and I was wrong :)
I'm not trying to be extra hard here, it's just that I have a lot of history with this patch...
So if you all have a reproducer, I would love to see it.
Oh, and the backports to the other kernel versions as well, you don't want to have to fix this again in a year when you all upgrade to a new release.
thanks,
greg k-h
On Tue, Jan 29, 2019 at 7:19 AM Greg KH gregkh@linuxfoundation.org wrote:
On Tue, Jan 29, 2019 at 06:35:48AM -0800, Guenter Roeck wrote:
On Mon, Jan 28, 2019 at 10:58 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
Wait, why is this needed?
We have syzcaller/syzbot hits, and there is a reproducer. Is this sufficient ?
Nice, where?
See below.
And why only for 4.4? What about 4.9 and 4.14? Do you want to upgrade and suddenly hit the same "bug" that you fixed before?
Good point. Sorry, that got lost. We are, after all, oly human. I'd ask Zubin to provide backports, or do it myself, but we'll have to resolve the issue you bring up below first.
There was a reason that I did not backport this to the stable tree when it was submitted, and that was because this was an odd race to ever hit. Are you hitting this in the real world without kobject deferred release enabled? And if so, are you hitting the WARN_ON that is added here?
I think we may need updated rules for stable. Many bug fixes are backported to stable releases without having been seen in the "real world" (whatever that means). At the same time we do see many races in the real world, many of them not fully understood. Our policy so far is to fix as many problems as possible if they are understood, in the hope that it fixes at least some of those problems seen in the field.
I don't have a problem with backporting this, but it went in as a "fixes a theoritical issue, and let's WARN_ON if it really ever is hit".
So I would like to see where we are hitting that WARN_ON, as it sounds like you found an easy-to-reproduce way to do it.
If there is a new rule that problems have to have been observed in the real world (ie without debugging options enabled, and without specific reproducer) before a patch is applied to stable, can we have that as generic rule that is not only selectively applied ?
It's not a specific rule, it's just that this specific patch went through a 30+ email chain of us arguing about it, so I really want to see why this is needed, and why Ben was right and I was wrong :)
I'm not trying to be extra hard here, it's just that I have a lot of history with this patch...
So if you all have a reproducer, I would love to see it.
Turns out you were already on Cc: for one of the reports:
https://b.corp.google.com/issues/112630408
I added you to the other:
https://b.corp.google.com/issues/112630534
The syzbot dashboard links in those bugs point to the reproducers.
Oh, and the backports to the other kernel versions as well, you don't want to have to fix this again in a year when you all upgrade to a new release.
For v4.14: The upstream patch (726e41097920) applies directly. For v4.9.y: The patch provided for v4.4.y also applies to v4.9.y.
Thanks, Guenter
On Tue, Jan 29, 2019 at 11:22 AM Guenter Roeck groeck@google.com wrote:
On Tue, Jan 29, 2019 at 7:19 AM Greg KH gregkh@linuxfoundation.org wrote:
On Tue, Jan 29, 2019 at 06:35:48AM -0800, Guenter Roeck wrote:
On Mon, Jan 28, 2019 at 10:58 PM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
Wait, why is this needed?
We have syzcaller/syzbot hits, and there is a reproducer. Is this sufficient ?
Nice, where?
See below.
And why only for 4.4? What about 4.9 and 4.14? Do you want to upgrade and suddenly hit the same "bug" that you fixed before?
Good point. Sorry, that got lost. We are, after all, oly human. I'd ask Zubin to provide backports, or do it myself, but we'll have to resolve the issue you bring up below first.
There was a reason that I did not backport this to the stable tree when it was submitted, and that was because this was an odd race to ever hit. Are you hitting this in the real world without kobject deferred release enabled? And if so, are you hitting the WARN_ON that is added here?
I think we may need updated rules for stable. Many bug fixes are backported to stable releases without having been seen in the "real world" (whatever that means). At the same time we do see many races in the real world, many of them not fully understood. Our policy so far is to fix as many problems as possible if they are understood, in the hope that it fixes at least some of those problems seen in the field.
I don't have a problem with backporting this, but it went in as a "fixes a theoritical issue, and let's WARN_ON if it really ever is hit".
So I would like to see where we are hitting that WARN_ON, as it sounds like you found an easy-to-reproduce way to do it.
If there is a new rule that problems have to have been observed in the real world (ie without debugging options enabled, and without specific reproducer) before a patch is applied to stable, can we have that as generic rule that is not only selectively applied ?
It's not a specific rule, it's just that this specific patch went through a 30+ email chain of us arguing about it, so I really want to see why this is needed, and why Ben was right and I was wrong :)
I'm not trying to be extra hard here, it's just that I have a lot of history with this patch...
So if you all have a reproducer, I would love to see it.
Turns out you were already on Cc: for one of the reports:
https://b.corp.google.com/issues/112630408
I added you to the other:
Public link:
https://groups.google.com/forum/#%21msg/syzkaller-bugs/LruHGgq_a-k/bd0RED9nA...
Unfortunately, that is the only one I found. There may be others - the internal links above suggest that the problem was reported several times - but I wasn't able to find it.
Guenter
The syzbot dashboard links in those bugs point to the reproducers.
Oh, and the backports to the other kernel versions as well, you don't want to have to fix this again in a year when you all upgrade to a new release.
For v4.14: The upstream patch (726e41097920) applies directly. For v4.9.y: The patch provided for v4.4.y also applies to v4.9.y.
Thanks, Guenter
On Mon, Jan 28, 2019 at 09:31:30AM -0800, Zubin Mithra wrote:
From: Benjamin Herrenschmidt benh@kernel.crashing.org
commit 726e41097920a73e4c7c33385dcc0debb1281e18 upstream
For devices with a class, we create a "glue" directory between the parent device and the new device with the class name.
This directory is never "explicitely" removed when empty however, this is left to the implicit sysfs removal done by kobject_release() when the object loses its last reference via kobject_put().
This is problematic because as long as it's not been removed from sysfs, it is still present in the class kset and in sysfs directory structure.
The presence in the class kset exposes a use after free bug fixed by the previous patch, but the presence in sysfs means that until the kobject is released, which can take a while (especially with kobject debugging), any attempt at re-creating such as binding a new device for that class/parent pair, will result in a sysfs duplicate file name error.
This fixes it by instead doing an explicit kobject_del() when the glue dir is empty, by keeping track of the number of child devices of the gluedir.
This is made easy by the fact that all glue dir operations are done with a global mutex, and there's already a function (cleanup_glue_dir) called in all the right places taking that mutex that can be enhanced for this. It appears that this was in fact the intent of the function, but the implementation was wrong.
Backport Note: kref_read() is not present in 4.4. Hence, use atomic_read(&kref.refcount) instead of kref_read(&kref).
Signed-off-by: Benjamin Herrenschmidt benh@kernel.crashing.org Acked-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Zubin Mithra zsm@chromium.org
drivers/base/core.c | 2 ++ include/linux/kobject.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)
Now queued up everywhere, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Guenter Roeck -
Sasha Levin -
Zubin Mithra