| Message ID | 20191116002802.6663-3-cyphar@cyphar.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | open: introduce openat2(2) syscall | expand |
On Sat, Nov 16, 2019 at 11:27:52AM +1100, Aleksa Sarai wrote: > + error = nd_jump_link(&path); > + if (error) > + path_put(&path); > + error = nd_jump_link(&ns_path); > + if (error) > + path_put(&ns_path); > + error = nd_jump_link(&path); > + if (error) > + path_put(&path); 3 calls. Exact same boilerplate in each to handle a failure case. Which spells "wrong calling conventions"; it's absolutely clear that we want that path_put() inside nd_jump_link(). The rule should be this: reference that used to be held in *path is consumed in any case. On success it goes into nd->path, on error it's just dropped, but in any case, the caller has the same refcounting environment to deal with. If you need the same boilerplate cleanup on failure again and again, the calling conventions are wrong and need to be fixed. And I'm not sure that int is the right return type here, to be honest. void * might be better - return ERR_PTR() or NULL, so that the value could be used as return value of ->get_link() that calls that thing.
On 2019-11-16, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Sat, Nov 16, 2019 at 11:27:52AM +1100, Aleksa Sarai wrote: > > + error = nd_jump_link(&path); > > + if (error) > > + path_put(&path); > > > + error = nd_jump_link(&ns_path); > > + if (error) > > + path_put(&ns_path); > > > + error = nd_jump_link(&path); > > + if (error) > > + path_put(&path); > > 3 calls. Exact same boilerplate in each to handle a failure case. > Which spells "wrong calling conventions"; it's absolutely clear > that we want that path_put() inside nd_jump_link(). > > The rule should be this: reference that used to be held in > *path is consumed in any case. On success it goes into > nd->path, on error it's just dropped, but in any case, the > caller has the same refcounting environment to deal with. > > If you need the same boilerplate cleanup on failure again and again, > the calling conventions are wrong and need to be fixed. Will do. > And I'm not sure that int is the right return type here, to be honest. > void * might be better - return ERR_PTR() or NULL, so that the value > could be used as return value of ->get_link() that calls that thing. I don't agree, given that the few callers of ns_get_path() are inconsistent with regards to whether they should use IS_ERR() or check for NULL, not to mention that "void *error" reads to me as being very odd given how common "int error" is throughout the kernel. There's also the "error == ERR_PTR(-EAGAIN)" checks which also read as being quite odd too. But the main motivating factor for changing it was that the one use where "void *" is useful (proc_ns_get_link) becomes needlessly ugly because of the "nd_jump_link() can return errors" change: error = ERR_PTR(nd_jump_link(&ns_path)); Or probably (if you don't want to rely on ERR_PTR(0) == NULL): int err = nd_jump_link(&ns_path); if (err) error = ERR_PTR(err);
diff --git a/fs/namei.c b/fs/namei.c index 671c3c1a3425..965a25b2e3df 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -859,7 +859,7 @@ static int nd_jump_root(struct nameidata *nd) * Helper to directly jump to a known parsed path from ->get_link, * caller must have taken a reference to path beforehand. */ -void nd_jump_link(struct path *path) +int nd_jump_link(struct path *path) { struct nameidata *nd = current->nameidata; path_put(&nd->path); @@ -867,6 +867,7 @@ void nd_jump_link(struct path *path) nd->path = *path; nd->inode = nd->path.dentry->d_inode; nd->flags |= LOOKUP_JUMPED; + return 0; } static inline void put_link(struct nameidata *nd) diff --git a/fs/proc/base.c b/fs/proc/base.c index ebea9501afb8..fecd5b4af607 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1626,8 +1626,9 @@ static const char *proc_pid_get_link(struct dentry *dentry, if (error) goto out; - nd_jump_link(&path); - return NULL; + error = nd_jump_link(&path); + if (error) + path_put(&path); out: return ERR_PTR(error); } diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index 08dd94df1a66..95e199fbad57 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -51,11 +51,18 @@ static const char *proc_ns_get_link(struct dentry *dentry, if (!task) return ERR_PTR(-EACCES); - if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) { - error = ns_get_path(&ns_path, task, ns_ops); - if (!error) - nd_jump_link(&ns_path); - } + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) + goto out; + + error = ns_get_path(&ns_path, task, ns_ops); + if (error) + goto out; + + error = nd_jump_link(&ns_path); + if (error) + path_put(&ns_path); + +out: put_task_struct(task); return ERR_PTR(error); } diff --git a/include/linux/namei.h b/include/linux/namei.h index 397a08ade6a2..758e9b47db6f 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -68,7 +68,7 @@ extern int follow_up(struct path *); extern struct dentry *lock_rename(struct dentry *, struct dentry *); extern void unlock_rename(struct dentry *, struct dentry *); -extern void nd_jump_link(struct path *path); +extern int __must_check nd_jump_link(struct path *path); static inline void nd_terminate_link(void *name, size_t len, size_t maxlen) { diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 45d13b6462aa..da045d0477a5 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -2455,16 +2455,20 @@ static const char *policy_get_link(struct dentry *dentry, { struct aa_ns *ns; struct path path; + int error; if (!dentry) return ERR_PTR(-ECHILD); + ns = aa_get_current_ns(); path.mnt = mntget(aafs_mnt); path.dentry = dget(ns_dir(ns)); - nd_jump_link(&path); + error = nd_jump_link(&path); + if (error) + path_put(&path); aa_put_ns(ns); - return NULL; + return ERR_PTR(error); } static int policy_readlink(struct dentry *dentry, char __user *buffer,
In preparation for LOOKUP_NO_MAGICLINKS, it's necessary to add the ability for nd_jump_link() to return an error which the corresponding get_link() caller must propogate back up to the VFS. Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> --- fs/namei.c | 3 ++- fs/proc/base.c | 5 +++-- fs/proc/namespaces.c | 17 ++++++++++++----- include/linux/namei.h | 2 +- security/apparmor/apparmorfs.c | 8 ++++++-- 5 files changed, 24 insertions(+), 11 deletions(-)