* [PATCH 1/3] namei: Fix use after free in kern_path_locked
2021-09-01 17:51 [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Stephen Brennan
@ 2021-09-01 17:51 ` Stephen Brennan
2021-09-01 17:51 ` [PATCH 2/3] namei: Standardize callers of filename_lookup() Stephen Brennan
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Stephen Brennan @ 2021-09-01 17:51 UTC (permalink / raw)
To: Alexander Viro, Jens Axboe, Dmitry Kadashev
Cc: Stephen Brennan, linux-fsdevel, linux-kernel
In 0ee50b47532a ("namei: change filename_parentat() calling
conventions"), filename_parentat() was made to always call putname() on
the filename before returning, and kern_path_locked() was migrated to
this calling convention. However, kern_path_locked() uses the "last"
parameter to lookup and potentially create a new dentry. The last
parameter contains the last component of the path and points within the
filename, which was recently freed at the end of filename_parentat().
Thus, when kern_path_locked() calls __lookup_hash(), it is using the
filename after it has already been freed.
In this case, filename_parentat() is fundamentally broken due to this
use after free. So, remove it, and rename __filename_parentat to
filename_parentat, migrating all callers. Adjust kern_path_locked to put
the filename once all users are done with it.
Fixes: 0ee50b47532a ("namei: change filename_parentat() calling conventions")
Link: https://lore.kernel.org/linux-fsdevel/YS9D4AlEsaCxLFV0@infradead.org/
Link: https://lore.kernel.org/linux-fsdevel/YS+csMTV2tTXKg3s@zeniv-ca.linux.org.uk/
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Reported-by: syzbot+fb0d60a179096e8c2731@syzkaller.appspotmail.com
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Co-authored-by: Dmitry Kadashev <dkadashev@gmail.com>
---
fs/namei.c | 47 ++++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 25 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index d049d3972695..f2af301cc79f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2514,9 +2514,10 @@ static int path_parentat(struct nameidata *nd, unsigned flags,
return err;
}
-static int __filename_parentat(int dfd, struct filename *name,
- unsigned int flags, struct path *parent,
- struct qstr *last, int *type)
+/* Note: this does not consume "name" */
+static int filename_parentat(int dfd, struct filename *name,
+ unsigned int flags, struct path *parent,
+ struct qstr *last, int *type)
{
int retval;
struct nameidata nd;
@@ -2538,30 +2539,24 @@ static int __filename_parentat(int dfd, struct filename *name,
return retval;
}
-static int filename_parentat(int dfd, struct filename *name,
- unsigned int flags, struct path *parent,
- struct qstr *last, int *type)
-{
- int retval = __filename_parentat(dfd, name, flags, parent, last, type);
-
- putname(name);
- return retval;
-}
-
/* does lookup, returns the object with parent locked */
struct dentry *kern_path_locked(const char *name, struct path *path)
{
+ struct filename *filename;
struct dentry *d;
struct qstr last;
int type, error;
- error = filename_parentat(AT_FDCWD, getname_kernel(name), 0, path,
- &last, &type);
- if (error)
- return ERR_PTR(error);
+ filename = getname_kernel(name);
+ error = filename_parentat(AT_FDCWD, filename, 0, path, &last, &type);
+ if (error) {
+ d = ERR_PTR(error);
+ goto out;
+ }
if (unlikely(type != LAST_NORM)) {
path_put(path);
- return ERR_PTR(-EINVAL);
+ d = ERR_PTR(-EINVAL);
+ goto out;
}
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
d = __lookup_hash(&last, path->dentry, 0);
@@ -2569,6 +2564,8 @@ struct dentry *kern_path_locked(const char *name, struct path *path)
inode_unlock(path->dentry->d_inode);
path_put(path);
}
+out:
+ putname(filename);
return d;
}
@@ -3634,7 +3631,7 @@ static struct dentry *__filename_create(int dfd, struct filename *name,
*/
lookup_flags &= LOOKUP_REVAL;
- error = __filename_parentat(dfd, name, lookup_flags, path, &last, &type);
+ error = filename_parentat(dfd, name, lookup_flags, path, &last, &type);
if (error)
return ERR_PTR(error);
@@ -3996,7 +3993,7 @@ int do_rmdir(int dfd, struct filename *name)
int type;
unsigned int lookup_flags = 0;
retry:
- error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
+ error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
if (error)
goto exit1;
@@ -4135,7 +4132,7 @@ int do_unlinkat(int dfd, struct filename *name)
struct inode *delegated_inode = NULL;
unsigned int lookup_flags = 0;
retry:
- error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
+ error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
if (error)
goto exit1;
@@ -4683,13 +4680,13 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
target_flags = 0;
retry:
- error = __filename_parentat(olddfd, from, lookup_flags, &old_path,
- &old_last, &old_type);
+ error = filename_parentat(olddfd, from, lookup_flags, &old_path,
+ &old_last, &old_type);
if (error)
goto put_names;
- error = __filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last,
- &new_type);
+ error = filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last,
+ &new_type);
if (error)
goto exit1;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] namei: Standardize callers of filename_lookup()
2021-09-01 17:51 [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Stephen Brennan
2021-09-01 17:51 ` [PATCH 1/3] namei: Fix use after free in kern_path_locked Stephen Brennan
@ 2021-09-01 17:51 ` Stephen Brennan
2021-09-01 17:51 ` [PATCH 3/3] namei: Standardize callers of filename_create() Stephen Brennan
2021-09-07 21:09 ` [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Al Viro
3 siblings, 0 replies; 10+ messages in thread
From: Stephen Brennan @ 2021-09-01 17:51 UTC (permalink / raw)
To: Alexander Viro; +Cc: Stephen Brennan, linux-fsdevel, linux-kernel
filename_lookup() has two variants, one which drops the caller's
reference to filename (filename_lookup), and one which does
not (__filename_lookup). This can be confusing as it's unusual to drop a
caller's reference. Remove filename_lookup, rename __filename_lookup
to filename_lookup, and convert all callers. The cost is a few slightly
longer functions, but the clarity is greater.
Link: https://lore.kernel.org/linux-fsdevel/YS+dstZ3xfcLxhoB@zeniv-ca.linux.org.uk/
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
fs/fs_parser.c | 1 -
fs/namei.c | 41 ++++++++++++++++++++++++-----------------
2 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 980d44fd3a36..3df07c0e32b3 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -165,7 +165,6 @@ int fs_lookup_param(struct fs_context *fc,
return invalf(fc, "%s: not usable as path", param->key);
}
- f->refcnt++; /* filename_lookup() drops our ref. */
ret = filename_lookup(param->dirfd, f, flags, _path, NULL);
if (ret < 0) {
errorf(fc, "%s: Lookup failure for '%s'", param->key, f->name);
diff --git a/fs/namei.c b/fs/namei.c
index f2af301cc79f..76871b7f127a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2467,7 +2467,7 @@ static int path_lookupat(struct nameidata *nd, unsigned flags, struct path *path
return err;
}
-static int __filename_lookup(int dfd, struct filename *name, unsigned flags,
+int filename_lookup(int dfd, struct filename *name, unsigned flags,
struct path *path, struct path *root)
{
int retval;
@@ -2488,15 +2488,6 @@ static int __filename_lookup(int dfd, struct filename *name, unsigned flags,
return retval;
}
-int filename_lookup(int dfd, struct filename *name, unsigned flags,
- struct path *path, struct path *root)
-{
- int retval = __filename_lookup(dfd, name, flags, path, root);
-
- putname(name);
- return retval;
-}
-
/* Returns 0 and nd will be valid on success; Retuns error, otherwise. */
static int path_parentat(struct nameidata *nd, unsigned flags,
struct path *parent)
@@ -2571,8 +2562,14 @@ struct dentry *kern_path_locked(const char *name, struct path *path)
int kern_path(const char *name, unsigned int flags, struct path *path)
{
- return filename_lookup(AT_FDCWD, getname_kernel(name),
- flags, path, NULL);
+ struct filename *filename;
+ int ret;
+
+ filename = getname_kernel(name);
+ ret = filename_lookup(AT_FDCWD, filename, flags, path, NULL);
+ putname(filename);
+ return ret;
+
}
EXPORT_SYMBOL(kern_path);
@@ -2588,10 +2585,15 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
const char *name, unsigned int flags,
struct path *path)
{
+ struct filename *filename;
struct path root = {.mnt = mnt, .dentry = dentry};
+ int ret;
+
+ filename = getname_kernel(name);
/* the first argument of filename_lookup() is ignored with root */
- return filename_lookup(AT_FDCWD, getname_kernel(name),
- flags , path, &root);
+ ret = filename_lookup(AT_FDCWD, filename, flags, path, &root);
+ putname(filename);
+ return ret;
}
EXPORT_SYMBOL(vfs_path_lookup);
@@ -2795,8 +2797,13 @@ int path_pts(struct path *path)
int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
struct path *path, int *empty)
{
- return filename_lookup(dfd, getname_flags(name, flags, empty),
- flags, path, NULL);
+ struct filename *filename;
+ int ret;
+
+ filename = getname_flags(name, flags, empty);
+ ret = filename_lookup(dfd, filename, flags, path, NULL);
+ putname(filename);
+ return ret;
}
EXPORT_SYMBOL(user_path_at_empty);
@@ -4421,7 +4428,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
if (flags & AT_SYMLINK_FOLLOW)
how |= LOOKUP_FOLLOW;
retry:
- error = __filename_lookup(olddfd, old, how, &old_path, NULL);
+ error = filename_lookup(olddfd, old, how, &old_path, NULL);
if (error)
goto out_putnames;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/3] namei: Standardize callers of filename_create()
2021-09-01 17:51 [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Stephen Brennan
2021-09-01 17:51 ` [PATCH 1/3] namei: Fix use after free in kern_path_locked Stephen Brennan
2021-09-01 17:51 ` [PATCH 2/3] namei: Standardize callers of filename_lookup() Stephen Brennan
@ 2021-09-01 17:51 ` Stephen Brennan
2021-09-07 20:13 ` Al Viro
2021-09-07 21:09 ` [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Al Viro
3 siblings, 1 reply; 10+ messages in thread
From: Stephen Brennan @ 2021-09-01 17:51 UTC (permalink / raw)
To: Alexander Viro; +Cc: Stephen Brennan, linux-fsdevel, linux-kernel
filename_create() has two variants, one which drops the caller's
reference to filename (filename_create) and one which does
not (__filename_create). This can be confusing as it's unusual to drop a
caller's reference. Remove filename_create, rename __filename_create
to filename_create, and convert all callers.
Link: https://lore.kernel.org/linux-fsdevel/f6238254-35bd-7e97-5b27-21050c745874@oracle.com/
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
fs/namei.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 76871b7f127a..ec76f533ee3e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3622,8 +3622,8 @@ struct file *do_file_open_root(const struct path *root,
return file;
}
-static struct dentry *__filename_create(int dfd, struct filename *name,
- struct path *path, unsigned int lookup_flags)
+static struct dentry *filename_create(int dfd, struct filename *name,
+ struct path *path, unsigned int lookup_flags)
{
struct dentry *dentry = ERR_PTR(-EEXIST);
struct qstr last;
@@ -3691,20 +3691,16 @@ static struct dentry *__filename_create(int dfd, struct filename *name,
return dentry;
}
-static inline struct dentry *filename_create(int dfd, struct filename *name,
- struct path *path, unsigned int lookup_flags)
-{
- struct dentry *res = __filename_create(dfd, name, path, lookup_flags);
-
- putname(name);
- return res;
-}
-
struct dentry *kern_path_create(int dfd, const char *pathname,
struct path *path, unsigned int lookup_flags)
{
- return filename_create(dfd, getname_kernel(pathname),
- path, lookup_flags);
+ struct filename *filename;
+ struct dentry *dentry;
+
+ filename = getname_kernel(pathname);
+ dentry = filename_create(dfd, filename, path, lookup_flags);
+ putname(filename);
+ return dentry;
}
EXPORT_SYMBOL(kern_path_create);
@@ -3720,7 +3716,13 @@ EXPORT_SYMBOL(done_path_create);
inline struct dentry *user_path_create(int dfd, const char __user *pathname,
struct path *path, unsigned int lookup_flags)
{
- return filename_create(dfd, getname(pathname), path, lookup_flags);
+ struct filename *filename;
+ struct dentry *dentry;
+
+ filename = getname(pathname);
+ dentry = filename_create(dfd, getname(pathname), path, lookup_flags);
+ putname(filename);
+ return dentry;
}
EXPORT_SYMBOL(user_path_create);
@@ -3801,7 +3803,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
if (error)
goto out1;
retry:
- dentry = __filename_create(dfd, name, &path, lookup_flags);
+ dentry = filename_create(dfd, name, &path, lookup_flags);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto out1;
@@ -3901,7 +3903,7 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
unsigned int lookup_flags = LOOKUP_DIRECTORY;
retry:
- dentry = __filename_create(dfd, name, &path, lookup_flags);
+ dentry = filename_create(dfd, name, &path, lookup_flags);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto out_putname;
@@ -4268,7 +4270,7 @@ int do_symlinkat(struct filename *from, int newdfd, struct filename *to)
goto out_putnames;
}
retry:
- dentry = __filename_create(newdfd, to, &path, lookup_flags);
+ dentry = filename_create(newdfd, to, &path, lookup_flags);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto out_putnames;
@@ -4432,7 +4434,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
if (error)
goto out_putnames;
- new_dentry = __filename_create(newdfd, new, &new_path,
+ new_dentry = filename_create(newdfd, new, &new_path,
(how & LOOKUP_REVAL));
error = PTR_ERR(new_dentry);
if (IS_ERR(new_dentry))
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] namei: Standardize callers of filename_create()
2021-09-01 17:51 ` [PATCH 3/3] namei: Standardize callers of filename_create() Stephen Brennan
@ 2021-09-07 20:13 ` Al Viro
2021-09-07 20:35 ` Stephen Brennan
0 siblings, 1 reply; 10+ messages in thread
From: Al Viro @ 2021-09-07 20:13 UTC (permalink / raw)
To: Stephen Brennan; +Cc: linux-fsdevel, linux-kernel
On Wed, Sep 01, 2021 at 10:51:43AM -0700, Stephen Brennan wrote:
> inline struct dentry *user_path_create(int dfd, const char __user *pathname,
> struct path *path, unsigned int lookup_flags)
> {
> - return filename_create(dfd, getname(pathname), path, lookup_flags);
> + struct filename *filename;
> + struct dentry *dentry;
> +
> + filename = getname(pathname);
> + dentry = filename_create(dfd, getname(pathname), path, lookup_flags);
> + putname(filename);
> + return dentry;
Leaks, obviously...
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] namei: Standardize callers of filename_create()
2021-09-07 20:13 ` Al Viro
@ 2021-09-07 20:35 ` Stephen Brennan
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Brennan @ 2021-09-07 20:35 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, linux-kernel
On 9/7/21 1:13 PM, Al Viro wrote:
> On Wed, Sep 01, 2021 at 10:51:43AM -0700, Stephen Brennan wrote:
>> inline struct dentry *user_path_create(int dfd, const char __user *pathname,
>> struct path *path, unsigned int lookup_flags)
>> {
>> - return filename_create(dfd, getname(pathname), path, lookup_flags);
>> + struct filename *filename;
>> + struct dentry *dentry;
>> +
>> + filename = getname(pathname);
>> + dentry = filename_create(dfd, getname(pathname), path, lookup_flags);
>> + putname(filename);
>> + return dentry;
>
> Leaks, obviously...
>
Ouch, thanks for the catch. I'll send v2 shortly.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] namei: fix use-after-free and adjust calling conventions
2021-09-01 17:51 [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Stephen Brennan
` (2 preceding siblings ...)
2021-09-01 17:51 ` [PATCH 3/3] namei: Standardize callers of filename_create() Stephen Brennan
@ 2021-09-07 21:09 ` Al Viro
2021-09-07 21:43 ` Stephen Brennan
2021-09-08 18:47 ` Stephen Brennan
3 siblings, 2 replies; 10+ messages in thread
From: Al Viro @ 2021-09-07 21:09 UTC (permalink / raw)
To: Stephen Brennan; +Cc: linux-fsdevel, linux-kernel
On Wed, Sep 01, 2021 at 10:51:40AM -0700, Stephen Brennan wrote:
> Drawing from the comments on the last two patches from me and Dmitry,
> the concensus is that __filename_parentat() is inherently buggy, and
> should be removed. But there's some nice consistency to the way that
> the other functions (filename_create, filename_lookup) are named which
> would get broken.
>
> I looked at the callers of filename_create and filename_lookup. All are
> small functions which are trivial to modify to include a putname(). It
> seems to me that adding a few more lines to these functions is a good
> traedoff for better clarity on lifetimes (as it's uncommon for functions
> to drop references to their parameters) and better consistency.
>
> This small series combines the UAF fix from me, and the removal of
> __filename_parentat() from Dmitry as patch 1. Then I standardize
> filename_create() and filename_lookup() and their callers.
For kern_path_locked() itself, I'd probably go for
static struct dentry *__kern_path_locked(struct filename *name, struct path *path)
{
struct dentry *d;
struct qstr last;
int type, error;
error = filename_parentat(AT_FDCWD, name, 0, path,
&last, &type);
if (error)
return ERR_PTR(error);
if (unlikely(type != LAST_NORM)) {
path_put(path);
return ERR_PTR(-EINVAL);
}
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
d = __lookup_hash(&last, path->dentry, 0);
if (IS_ERR(d)) {
inode_unlock(path->dentry->d_inode);
path_put(path);
}
return d;
}
static struct dentry *kern_path_locked(const char *name, struct path *path)
{
struct filename *filename = getname_kernel(name);
struct dentry *res = __kern_path_locked(filename, path);
putname(filename);
return res;
}
instead of that messing with gotos - and split renaming from fix in that
commit. In 3/3 you have a leak; trivial to fix, fortunately.
Another part I really dislike in that area (not your fault, obviously)
is
void putname(struct filename *name)
{
if (IS_ERR_OR_NULL(name))
return;
in mainline right now. Could somebody explain when the hell has NULL
become a possibility here? OK, I buy putname(ERR_PTR(...)) being
a no-op, but IME every sodding time we mixed NULL and ERR_PTR() in
an API we ended up with headache later.
IS_ERR_OR_NULL() is almost always wrong. NULL as argument
for destructor makes sense when constructor can fail with NULL;
not the case here.
How about the variant in vfs.git#misc.namei?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] namei: fix use-after-free and adjust calling conventions
2021-09-07 21:09 ` [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Al Viro
@ 2021-09-07 21:43 ` Stephen Brennan
2021-09-07 21:54 ` Al Viro
2021-09-08 18:47 ` Stephen Brennan
1 sibling, 1 reply; 10+ messages in thread
From: Stephen Brennan @ 2021-09-07 21:43 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, linux-kernel
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Wed, Sep 01, 2021 at 10:51:40AM -0700, Stephen Brennan wrote:
>> Drawing from the comments on the last two patches from me and Dmitry,
>> the concensus is that __filename_parentat() is inherently buggy, and
>> should be removed. But there's some nice consistency to the way that
>> the other functions (filename_create, filename_lookup) are named which
>> would get broken.
>>
>> I looked at the callers of filename_create and filename_lookup. All are
>> small functions which are trivial to modify to include a putname(). It
>> seems to me that adding a few more lines to these functions is a good
>> traedoff for better clarity on lifetimes (as it's uncommon for functions
>> to drop references to their parameters) and better consistency.
>>
>> This small series combines the UAF fix from me, and the removal of
>> __filename_parentat() from Dmitry as patch 1. Then I standardize
>> filename_create() and filename_lookup() and their callers.
>
> For kern_path_locked() itself, I'd probably go for
>
> static struct dentry *__kern_path_locked(struct filename *name, struct path *path)
> {
> struct dentry *d;
> struct qstr last;
> int type, error;
>
> error = filename_parentat(AT_FDCWD, name, 0, path,
> &last, &type);
> if (error)
> return ERR_PTR(error);
> if (unlikely(type != LAST_NORM)) {
> path_put(path);
> return ERR_PTR(-EINVAL);
> }
> inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> d = __lookup_hash(&last, path->dentry, 0);
> if (IS_ERR(d)) {
> inode_unlock(path->dentry->d_inode);
> path_put(path);
> }
> return d;
> }
>
> static struct dentry *kern_path_locked(const char *name, struct path *path)
> {
> struct filename *filename = getname_kernel(name);
> struct dentry *res = __kern_path_locked(filename, path);
>
> putname(filename);
> return res;
> }
>
> instead of that messing with gotos - and split renaming from fix in that
> commit. In 3/3 you have a leak; trivial to fix, fortunately.
Got it. My v2 crossed paths with your message here, it only fixes the
leak but not the kern_path_locked() change and split. Please ignore it
and I'll adjust patch 1 for v3.
>
> Another part I really dislike in that area (not your fault, obviously)
> is
>
> void putname(struct filename *name)
> {
> if (IS_ERR_OR_NULL(name))
> return;
>
> in mainline right now. Could somebody explain when the hell has NULL
> become a possibility here? OK, I buy putname(ERR_PTR(...)) being
> a no-op, but IME every sodding time we mixed NULL and ERR_PTR() in
> an API we ended up with headache later.
From the links in the blame it seems this was suggested by Linus
here[1]. The core frustration having been with the state of
__filename_create() and friends freeing filenames at different times
depending on whether an error occurred.
[1] https://lore.kernel.org/io-uring/CAHk-=wgCac9hBsYzKMpHk0EbLgQaXR=OUAjHaBtaY+G8A9KhFg@mail.gmail.com/
Thanks,
Stephen
> IS_ERR_OR_NULL() is almost always wrong. NULL as argument
> for destructor makes sense when constructor can fail with NULL;
> not the case here.
>
> How about the variant in vfs.git#misc.namei?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] namei: fix use-after-free and adjust calling conventions
2021-09-07 21:43 ` Stephen Brennan
@ 2021-09-07 21:54 ` Al Viro
0 siblings, 0 replies; 10+ messages in thread
From: Al Viro @ 2021-09-07 21:54 UTC (permalink / raw)
To: Stephen Brennan; +Cc: linux-fsdevel, linux-kernel
On Tue, Sep 07, 2021 at 02:43:48PM -0700, Stephen Brennan wrote:
> >From the links in the blame it seems this was suggested by Linus
> here[1]. The core frustration having been with the state of
> __filename_create() and friends freeing filenames at different times
> depending on whether an error occurred.
Sure, but that's an argument for IS_ERR(), not the IS_ERR_OR_NULL() shite...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] namei: fix use-after-free and adjust calling conventions
2021-09-07 21:09 ` [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Al Viro
2021-09-07 21:43 ` Stephen Brennan
@ 2021-09-08 18:47 ` Stephen Brennan
1 sibling, 0 replies; 10+ messages in thread
From: Stephen Brennan @ 2021-09-08 18:47 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, linux-kernel
Al Viro <viro@zeniv.linux.org.uk> writes:
> [snip]
> Another part I really dislike in that area (not your fault, obviously)
> is
>
> void putname(struct filename *name)
> {
> if (IS_ERR_OR_NULL(name))
> return;
>
> in mainline right now. Could somebody explain when the hell has NULL
> become a possibility here? OK, I buy putname(ERR_PTR(...)) being
> a no-op, but IME every sodding time we mixed NULL and ERR_PTR() in
> an API we ended up with headache later.
>
> IS_ERR_OR_NULL() is almost always wrong. NULL as argument
> for destructor makes sense when constructor can fail with NULL;
> not the case here.
>
> How about the variant in vfs.git#misc.namei?
I went and looked through the changelog of fs/namei.c since this was
changed and don't see anything setting a filename NULL, so it seems safe
and good to me. I couldn't check *every* user of filename but the change
was only two months ago. Feel free to use my r-b for that commit if you
want.
Reviewed-by: Stephen Brennan <stephen.s.brennan@oracle.com>
^ permalink raw reply [flat|nested] 10+ messages in thread