| Message ID | 20181112095632.69114-2-paolo.valente@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | unify the interface of the proportional-share policy in blkio/io | expand |
On Mon, Nov 12, 2018 at 10:56:21AM +0100, Paolo Valente wrote: > From: Angelo Ruocco <angeloruocco90@gmail.com> > > The kernfs pseudo file system doesn't export any function to only find > a node by name, without also getting a reference on it. > But in some cases it is useful to just locate a kernfs node, while > using it or not depends on some other condition. > > This commit adds a function to just look for a node, without getting > a reference on it. Eeek, that sounds really bad. So you save off a pointer to something, and have no idea if that pointer now really is valid or not? It can instantly disappear right afterwards. This feels wrong, what is the problem of having a properly reference counted object passed back to you that you have to create a dangerous function like this? greg k-h
On Mon, Nov 12, 2018 at 04:28:40AM -0800, Greg Kroah-Hartman wrote: > On Mon, Nov 12, 2018 at 10:56:21AM +0100, Paolo Valente wrote: > > From: Angelo Ruocco <angeloruocco90@gmail.com> > > > > The kernfs pseudo file system doesn't export any function to only find > > a node by name, without also getting a reference on it. > > But in some cases it is useful to just locate a kernfs node, while > > using it or not depends on some other condition. > > > > This commit adds a function to just look for a node, without getting > > a reference on it. > > Eeek, that sounds really bad. So you save off a pointer to something, > and have no idea if that pointer now really is valid or not? It can > instantly disappear right afterwards. > > This feels wrong, what is the problem of having a properly reference > counted object passed back to you that you have to create a dangerous > function like this? I agree with Greg, this function is dangerous. What's wrong with using find_get and then doing a put once you successfully take it over, or fail to take it over?
> Il giorno 12 nov 2018, alle ore 13:28, Greg Kroah-Hartman <gregkh@linuxfoundation.org> ha scritto: > > On Mon, Nov 12, 2018 at 10:56:21AM +0100, Paolo Valente wrote: >> From: Angelo Ruocco <angeloruocco90@gmail.com> >> >> The kernfs pseudo file system doesn't export any function to only find >> a node by name, without also getting a reference on it. >> But in some cases it is useful to just locate a kernfs node, while >> using it or not depends on some other condition. >> >> This commit adds a function to just look for a node, without getting >> a reference on it. > > Eeek, that sounds really bad. So you save off a pointer to something, > and have no idea if that pointer now really is valid or not? It can > instantly disappear right afterwards. > Hi Greg, that function is invoked only in functions executed with cgroup_mutex held. This guarantees that nothing disappears or becomes inconsistent. That's why we decided to go for this optimization, instead of doing useless gets&puts pairs. Still, I'm not expert enough to state whether it is impossible that, once we have defined that function, it may then get used in some unsafe way. So, I seem to see two options: 1) Add a comment on the function, saying that cgroup_mutex must be held while invoking it (I guess you won't like this one). 2) Do not define such a new function, and, in the other patches, use the already-available find_and_get. Looking forward to your feedback (or of other knowledgeable people on this issue) before proceeding to a rebased V2, Paolo > This feels wrong, what is the problem of having a properly reference > counted object passed back to you that you have to create a dangerous > function like this? > > greg k-h
On Tue, Nov 13, 2018 at 06:53:59PM +0100, Paolo Valente wrote: > > > > Il giorno 12 nov 2018, alle ore 13:28, Greg Kroah-Hartman <gregkh@linuxfoundation.org> ha scritto: > > > > On Mon, Nov 12, 2018 at 10:56:21AM +0100, Paolo Valente wrote: > >> From: Angelo Ruocco <angeloruocco90@gmail.com> > >> > >> The kernfs pseudo file system doesn't export any function to only find > >> a node by name, without also getting a reference on it. > >> But in some cases it is useful to just locate a kernfs node, while > >> using it or not depends on some other condition. > >> > >> This commit adds a function to just look for a node, without getting > >> a reference on it. > > > > Eeek, that sounds really bad. So you save off a pointer to something, > > and have no idea if that pointer now really is valid or not? It can > > instantly disappear right afterwards. > > > > Hi Greg, > that function is invoked only in functions executed with cgroup_mutex > held. This guarantees that nothing disappears or becomes > inconsistent. That's why we decided to go for this optimization, > instead of doing useless gets&puts pairs. Still, I'm not expert > enough to state whether it is impossible that, once we have defined > that function, it may then get used in some unsafe way. I can guarantee once you define that function, it will be used in an unsafe way :( So just don't create it, use the put calls, it's fast and should never be a performance issue. > So, I seem to see two options: > 1) Add a comment on the function, saying that cgroup_mutex must be > held while invoking it (I guess you won't like this one). Nope, do not create it. > 2) Do not define such a new function, and, in the other patches, use > the already-available find_and_get. Yes, please do that. thanks, greg k-h
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 4ca0b5c18192..0003c665869d 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -892,6 +892,19 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent, return parent; } +struct kernfs_node *kernfs_find(struct kernfs_node *parent, + const unsigned char *name) +{ + struct kernfs_node *kn; + + mutex_lock(&kernfs_mutex); + kn = kernfs_find_ns(parent, name, NULL); + mutex_unlock(&kernfs_mutex); + + return kn; +} +EXPORT_SYMBOL_GPL(kernfs_find); + /** * kernfs_find_and_get_ns - find and get kernfs_node with the given name * @parent: kernfs_node to search under diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h index 5b36b1287a5a..26e69d285964 100644 --- a/include/linux/kernfs.h +++ b/include/linux/kernfs.h @@ -309,6 +309,8 @@ void pr_cont_kernfs_path(struct kernfs_node *kn); struct kernfs_node *kernfs_get_parent(struct kernfs_node *kn); struct kernfs_node *kernfs_find_and_get_ns(struct kernfs_node *parent, const char *name, const void *ns); +struct kernfs_node *kernfs_find(struct kernfs_node *parent, + const unsigned char *name); struct kernfs_node *kernfs_walk_and_get_ns(struct kernfs_node *parent, const char *path, const void *ns); void kernfs_get(struct kernfs_node *kn); @@ -391,6 +393,11 @@ static inline struct kernfs_node * kernfs_find_and_get_ns(struct kernfs_node *parent, const char *name, const void *ns) { return NULL; } + +static inline struct kernfs_node * +kernfs_find(struct kernfs_node *parent, const char *name) +{ return NULL; } + static inline struct kernfs_node * kernfs_walk_and_get_ns(struct kernfs_node *parent, const char *path, const void *ns)