Messages in this thread | | | Date | Fri, 18 Mar 2022 00:07:21 +0000 | From | Al Viro <> | Subject | Re: [RESEND PATCH v7 7/8] kernfs: Replace per-fs rwsem with hashed rwsems. |
| |
On Thu, Mar 17, 2022 at 06:26:11PM +1100, Imran Khan wrote:
> diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c > index 9d4103602554..cbdd1be5f0a8 100644 > --- a/fs/kernfs/symlink.c > +++ b/fs/kernfs/symlink.c > @@ -113,12 +113,19 @@ static int kernfs_getlink(struct inode *inode, char *path) > struct kernfs_node *kn = inode->i_private; > struct kernfs_node *parent = kn->parent; > struct kernfs_node *target = kn->symlink.target_kn; > - struct rw_semaphore *rwsem; > + struct kernfs_rwsem_token token; > int error; > > - rwsem = kernfs_down_read(parent); > + /** > + * Lock both parent and target, to avoid their movement > + * or removal in the middle of path construction. > + * If a competing remove or rename for parent or target > + * wins, it will be reflected in result returned from > + * kernfs_get_target_path. > + */ > + kernfs_down_read_double_nodes(target, parent, &token); > error = kernfs_get_target_path(parent, target, path); > - kernfs_up_read(rwsem); > + kernfs_up_read_double_nodes(target, parent, &token); > > return error; > }
No. Read through the kernfs_get_target_path(). Why would locking these two specific nodes be sufficient for anything useful? That code relies upon ->parent of *many* nodes being stable. Which is not going to be guaranteed by anything of that sort.
And it's not just "we might get garbage if we race" - it's "we might walk into kfree'd object and proceed to walk the pointer chain".
Or have this loop kn = target; while (kn->parent && kn != base) { len += strlen(kn->name) + 1; kn = kn->parent; } see the names that are not identical to what we see in kn = target; while (kn->parent && kn != base) { int slen = strlen(kn->name);
len -= slen; memcpy(s + len, kn->name, slen); if (len) s[--len] = '/';
kn = kn->parent; } done later in the same function. With obvious unpleasant effects. Or a different set of nodes, for that matter.
This code really depends upon the tree being stable. No renames of any sort allowed during that thing.
| |