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 <[email protected]>
---
fs/proc/proc_sysctl.c | 1 -
1 file changed, 1 deletion(-)
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;
}
--
1.8.3.1
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 <[email protected]> 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