| Message ID | 1545444741-31694-1-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | proc: sysctl: fix double drop_sysctl_table() | expand |
On Sat, Dec 22, 2018 at 10:12:21AM +0800, Yafang Shao wrote: > The callers of insert_header() will drop sysctl table whatever > insert_header() successes or fails. > So we can't call drop_sysctl_table() in insert_header(). > > The call sites of insert_header() are all in fs/proc/proc_sysctl.c. Except that at least insert_links() does this: ... core_parent->header.nreg++; ... err = insert_header(core_parent, links); if (err) kfree(links); out: drop_sysctl_table(&core_parent->header); with that drop clearly paired with explicit increment of nreg. So your patch appears to break at least that one. Looking at the rest of callers... __register_sysctl_table() demonstrates the same pattern - explicit increment of nreg, then insert_header(), then *unconditional* drop undoing that increment. The third and last caller (get_subdir()) appears to be different. We do insert_header(), if it succeeds we bump nreg on new and unconditionally drop a reference to dir, as well as new. Let's look at the callers of that sucker. /* Reference moved down the diretory tree get_subdir */ dir->header.nreg++; spin_unlock(&sysctl_lock); /* Find the directory for the ctl_table */ for (name = path; name; name = nextname) { int namelen; nextname = strchr(name, '/'); if (nextname) { namelen = nextname - name; nextname++; } else { namelen = strlen(name); } if (namelen == 0) continue; dir = get_subdir(dir, name, namelen); if (IS_ERR(dir)) goto fail; } spin_lock(&sysctl_lock); if (insert_header(dir, header)) goto fail_put_dir_locked; Aha... So we are traversing a tree and it smells like get_subdir() expects dir to be pinned by the caller, drops that reference and either fails or returns the next tree node pinned. _IF_ that matches the behaviour of get_subdir(), the code above makes sense - grab dir for each non-empty pathname component next = get_subdir(dir, component) // dir got dropped if get_subdir has failed goto fail // next got grabbed dir = next insert_header(dir, header) drop dir if insert_header was successful return header fail: // all references grabbed by the above are dropped by now So let's look at get_subdir() from refcounting POV (ignoring the locking for now): subdir = find_subdir(dir, name, namelen); if (!IS_ERR(subdir)) goto found; if (PTR_ERR(subdir) != -ENOENT) goto failed; yeccchhhhhhh.... What's wrong with if (subdir == ERR_PTR(-ENOENT))? Anyway, find_subdir() appears to be refcounting-neutral. new = new_dir(set, name, namelen); OK, looks like we are creating a new object here. What's the value of .nreg in it? new_dir() itself doesn't seem to set it, but the thing it calls at the end (init_header()) does set it to 1. OK, so we'd got a reference to new object, our counter being 1. On failure it appears to return NULL. OK... subdir = ERR_PTR(-ENOMEM); if (!new) goto failed; right, so if allocation in new_dir() has failed, we are buggering off to the same 'failed' label as on other exits. In failure case new_dir() is refcounting-neutral, so we are in the same environment. /* Was the subdir added while we dropped the lock? */ subdir = find_subdir(dir, name, namelen); if (!IS_ERR(subdir)) goto found; if (PTR_ERR(subdir) != -ENOENT) goto failed; Interesting... So we can get to 'failed' in some cases when new_dir() has not failed. /* Nope. Use the our freshly made directory entry. */ err = insert_header(dir, &new->header); subdir = ERR_PTR(err); if (err) goto failed; Looks like it expects insert_header() to be refcounting-neutral in failure case... subdir = new; found: subdir->header.nreg++; OK, we can get here from 3 places: * subdir found by lookup before we even tried to allocate new. new is NULL, subdir has just gotten a reference grabbed. * subdir found by re-lookup after new had been allocated. new is non-NULL and we are holding one reference to it, subdir has just gotten a reference grabbed and it's not equal to new. * new got successfully inserted into dir; subdir is equal to new and we'd just grabbed the second reference to it. Looks like in all those cases we have a reference grabbed on subdir *and* a reference grabbed on new (if non-NULL). Covers all 3 cases.,, failed: ... and now we rejoin the failure paths (a lousy label name, that - we get here on success as well). if (IS_ERR(subdir)) whine drop reference to dir (the one that caller had grabbed for us) if new is not NULL drop reference to new. return subdir. OK, in success case we have ended up with the total effect drop dir grab subdir Holds in all 3 cases above. On failure, we have dropped the reference grabbed by new_dir (if any) and dropped dir. That matches the behaviour guessed by the look of the caller, and it definitely expects insert_header() to be refcounting-neutral on failures. And AFAICS, insert_header() is that. Before your patch, that is... The bottom line is, proposed behaviour change appears to be broken for all callers. NAK. PS: I was going to add that get_subdir() was in bad need of comments, until I actually looked around. And right in front of that function we have this: /** * get_subdir - find or create a subdir with the specified name. * @dir: Directory to create the subdirectory in * @name: The name of the subdirectory to find or create * @namelen: The length of name * * Takes a directory with an elevated reference count so we know that * if we drop the lock the directory will not go away. Upon success * the reference is moved from @dir to the returned subdirectory. * Upon error an error code is returned and the reference on @dir is * simply dropped. */ So it *IS* documented, description does match the behaviour and I should've bloody well checked if it's there first. And verified that it does match the code, of course, but that's generally simpler than deriving the behaviour from code and trying to come up with sane description of such.
On Sat, Dec 22, 2018 at 11:17 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Sat, Dec 22, 2018 at 10:12:21AM +0800, Yafang Shao wrote: > > The callers of insert_header() will drop sysctl table whatever > > insert_header() successes or fails. > > So we can't call drop_sysctl_table() in insert_header(). > > > > The call sites of insert_header() are all in fs/proc/proc_sysctl.c. > > Except that at least insert_links() does this: > > ... > core_parent->header.nreg++; > ... > err = insert_header(core_parent, links); > if (err) > kfree(links); > out: > drop_sysctl_table(&core_parent->header); > with that drop clearly paired with explicit increment of nreg. So your > patch appears to break at least that one. > > Looking at the rest of callers... __register_sysctl_table() demonstrates > the same pattern - explicit increment of nreg, then insert_header(), > then *unconditional* drop undoing that increment. > > The third and last caller (get_subdir()) appears to be different. > We do insert_header(), if it succeeds we bump nreg on new and > unconditionally drop a reference to dir, as well as new. > > Let's look at the callers of that sucker. > > /* Reference moved down the diretory tree get_subdir */ > dir->header.nreg++; > spin_unlock(&sysctl_lock); > > /* Find the directory for the ctl_table */ > for (name = path; name; name = nextname) { > int namelen; > nextname = strchr(name, '/'); > if (nextname) { > namelen = nextname - name; > nextname++; > } else { > namelen = strlen(name); > } > if (namelen == 0) > continue; > > dir = get_subdir(dir, name, namelen); > if (IS_ERR(dir)) > goto fail; > } > > spin_lock(&sysctl_lock); > if (insert_header(dir, header)) > goto fail_put_dir_locked; > > Aha... So we are traversing a tree and it smells like get_subdir() > expects dir to be pinned by the caller, drops that reference and > either fails or returns the next tree node pinned. > > _IF_ that matches the behaviour of get_subdir(), the code above > makes sense - > grab dir > for each non-empty pathname component > next = get_subdir(dir, component) > // dir got dropped > if get_subdir has failed > goto fail > // next got grabbed > dir = next > insert_header(dir, header) > drop dir > if insert_header was successful > return header > fail: > // all references grabbed by the above are dropped by now > > So let's look at get_subdir() from refcounting POV (ignoring the locking > for now): > subdir = find_subdir(dir, name, namelen); > if (!IS_ERR(subdir)) > goto found; > if (PTR_ERR(subdir) != -ENOENT) > goto failed; > yeccchhhhhhh.... What's wrong with if (subdir == ERR_PTR(-ENOENT))? > Anyway, find_subdir() appears to be refcounting-neutral. > new = new_dir(set, name, namelen); > OK, looks like we are creating a new object here. What's the value of .nreg > in it? new_dir() itself doesn't seem to set it, but the thing it calls at > the end (init_header()) does set it to 1. OK, so we'd got a reference to > new object, our counter being 1. On failure it appears to return NULL. > OK... > subdir = ERR_PTR(-ENOMEM); > if (!new) > goto failed; > right, so if allocation in new_dir() has failed, we are buggering off to the > same 'failed' label as on other exits. In failure case new_dir() is > refcounting-neutral, so we are in the same environment. > /* Was the subdir added while we dropped the lock? */ > subdir = find_subdir(dir, name, namelen); > if (!IS_ERR(subdir)) > goto found; > if (PTR_ERR(subdir) != -ENOENT) > goto failed; > Interesting... So we can get to 'failed' in some cases when new_dir() > has not failed. > /* Nope. Use the our freshly made directory entry. */ > err = insert_header(dir, &new->header); > subdir = ERR_PTR(err); > if (err) > goto failed; > Looks like it expects insert_header() to be refcounting-neutral in failure > case... > subdir = new; > found: > subdir->header.nreg++; > > OK, we can get here from 3 places: > * subdir found by lookup before we even tried to allocate new. > new is NULL, subdir has just gotten a reference grabbed. > * subdir found by re-lookup after new had been allocated. > new is non-NULL and we are holding one reference to it, subdir has > just gotten a reference grabbed and it's not equal to new. > * new got successfully inserted into dir; subdir is equal > to new and we'd just grabbed the second reference to it. > > Looks like in all those cases we have a reference grabbed on subdir > *and* a reference grabbed on new (if non-NULL). Covers all 3 cases.,, > > failed: > ... and now we rejoin the failure paths (a lousy label name, that - we > get here on success as well). > if (IS_ERR(subdir)) > whine > drop reference to dir (the one that caller had grabbed for us) > if new is not NULL > drop reference to new. > return subdir. > > OK, in success case we have ended up with the total effect > drop dir > grab subdir > Holds in all 3 cases above. On failure, we have dropped the reference > grabbed by new_dir (if any) and dropped dir. That matches the behaviour > guessed by the look of the caller, and it definitely expects > insert_header() to be refcounting-neutral on failures. > > And AFAICS, insert_header() is that. Before your patch, that is... > > The bottom line is, proposed behaviour change appears to be broken for all > callers. > > NAK. > > PS: I was going to add that get_subdir() was in bad need of comments, until > I actually looked around. And right in front of that function we have this: > /** > * get_subdir - find or create a subdir with the specified name. > * @dir: Directory to create the subdirectory in > * @name: The name of the subdirectory to find or create > * @namelen: The length of name > * > * Takes a directory with an elevated reference count so we know that > * if we drop the lock the directory will not go away. Upon success > * the reference is moved from @dir to the returned subdirectory. > * Upon error an error code is returned and the reference on @dir is > * simply dropped. > */ > > So it *IS* documented, description does match the behaviour and > I should've bloody well checked if it's there first. And verified > that it does match the code, of course, but that's generally > simpler than deriving the behaviour from code and trying to come > up with sane description of such. Many thanks for your detailed explanation! I undanstand it. Thanks Yafang
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 4d598a3..5eddca4 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -241,7 +241,6 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) if (header->ctl_table == sysctl_mount_point) clear_empty_dir(dir); header->parent = NULL; - drop_sysctl_table(&dir->header); return err; }
The callers of insert_header() will drop sysctl table whatever insert_header() successes or fails. So we can't call drop_sysctl_table() in insert_header(). The call sites of insert_header() are all in fs/proc/proc_sysctl.c. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- fs/proc/proc_sysctl.c | 1 - 1 file changed, 1 deletion(-)