Skip to contentSkip to conversation

proc/sysctl: don't return ENOMEM on lookup when a table is unregistering

PATCH Superseded Ivan Delalande 2 participants 4 messages Dec 13, 2018 – Dec 14, 2018

Subsystems (3)
VFS Proc filesystem Proc sysctl
lore
Share & tools
lore

Ivan Delalande

author
#0 lore raw

PATCH 1/1 proc/sysctl: don't return ENOMEM on lookup when a table is unregistering

proc_sys_lookup can fail with ENOMEM instead of ENOENT when the corresponding sysctl table is being unregistered. In our case we see this upon opening /proc/sys/net/*/conf files while network interfaces are being removed, which confuses our configuration daemon.

The problem was successfully reproduced and this fix tested on v4.9.122 and v4.20-rc6.

Fixes ace0c791e6c3 ("proc/sysctl: Don't grab i_lock under sysctl_lock.")
Cc [email protected]
Signed-off-by Ivan Delalande <[email protected]>
---
 fs/proc/proc_sysctl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
fs/proc/proc_sysctl.c +5 -4
index 89921a0d2ebb..834be5bc3d07 100644
@@ -474,7 +474,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	if (unlikely(head->unregistering)) {
 		spin_unlock(&sysctl_lock);
 		iput(inode);
-		inode = NULL;
+		inode = ERR_PTR(-ENOENT);
 		goto out;
 	}
 	ei->sysctl = head;

@@ -549,10 +549,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
 			goto out;
 	}
 
-	err = ERR_PTR(-ENOMEM);
 	inode = proc_sys_make_inode(dir->i_sb, h ? h : head, p);
-	if (!inode)
+	if (IS_ERR_OR_NULL(inode)) {
+		err = inode ? ERR_CAST(inode) : ERR_PTR(-ENOMEM);
 		goto out;
+	}
 
 	d_set_d_op(dentry, &proc_sys_dentry_operations);
 	err = d_splice_alias(inode, dentry);

@@ -685,7 +686,7 @@ static bool proc_sys_fill_cache(struct file *file,
 		if (d_in_lookup(child)) {
 			struct dentry *res;
 			inode = proc_sys_make_inode(dir->d_sb, head, table);
-			if (!inode) {
+			if (IS_ERR_OR_NULL(inode)) {
 				d_lookup_done(child);
 				dput(child);
 				return false;
Signature · 1 line
2.20.0
maintainer
#1 lore raw
Quoted 29 lines · Ivan Delalande

proc_sys_lookup can fail with ENOMEM instead of ENOENT when the corresponding sysctl table is being unregistered. In our case we see this upon opening /proc/sys/net/*/conf files while network interfaces are being removed, which confuses our configuration daemon.

The problem was successfully reproduced and this fix tested on v4.9.122 and v4.20-rc6.

Fixes ace0c791e6c3 ("proc/sysctl: Don't grab i_lock under sysctl_lock.")
Cc [email protected]
Signed-off-by Ivan Delalande <[email protected]>
---
 fs/proc/proc_sysctl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
fs/proc/proc_sysctl.c +1 -1
index 89921a0d2ebb..834be5bc3d07 100644
@@ -474,7 +474,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	if (unlikely(head->unregistering)) {
 		spin_unlock(&sysctl_lock);
 		iput(inode);
-		inode = NULL;
+		inode = ERR_PTR(-ENOENT);
 		goto out;
 	}
 	ei->sysctl = head;

fs/proc/proc_sysctl.c static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry, · line 554

Quoted 8 lines
fs/proc/proc_sysctl.c +2 -2
@@ -549,10 +549,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
 			goto out;
 	}
 
-	err = ERR_PTR(-ENOMEM);
 	inode = proc_sys_make_inode(dir->i_sb, h ? h : head, p);
-	if (!inode)
+	if (IS_ERR_OR_NULL(inode)) {
+		err = inode ? ERR_CAST(inode) : ERR_PTR(-ENOMEM);

*gags*

If you want to return specific errors, do just that and for pity sake, do *NOT* invent such hybrids. "Pointer to object on success, ERR_PTR(-E...) on failure, NULL means ERR_PTR(-ENOMEM)" is a bitch to reason about and prone to breakage.

"Return NULL on error" and "return ERR_PTR() on error" do not mix. Just make proc_sys_make_inode() return ERR_PTR(-ENOMEM) on allocation failures and update the callers (as you have to, anyway).

IS_ERR_OR_NULL() is usually a sign of bad calling conventions and it certainly is just that in this case.

Just do

        inode = new_inode(sb);
        if (!inode)
                return ERR_PTR(-ENOMEM);

in there, in addition to your return of ERR_PTR(-ENOENT), and lose those IS_ERR_OR_NULL() things. Make those IS_ERR() and turn the assignment to err into straight ERR_CAST(). All there is to it...

author
#2 lore raw

PATCH v2 1/1 proc/sysctl: don't return ENOMEM on lookup when a table is unregistering

proc_sys_lookup can fail with ENOMEM instead of ENOENT when the corresponding sysctl table is being unregistered. In our case we see this upon opening /proc/sys/net/*/conf files while network interfaces are being deleted, which confuses our configuration daemon.

The problem was successfully reproduced and this fix tested on v4.9.122 and v4.20-rc6.

v2: return ERR_PTRs in all cases when proc_sys_make_inode fails instead of mixing them with NULL. Thanks Al Viro for the feedback.

Fixes ace0c791e6c3 ("proc/sysctl: Don't grab i_lock under sysctl_lock.")
Cc [email protected]
Signed-off-by Ivan Delalande <[email protected]>
---
 fs/proc/proc_sysctl.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
fs/proc/proc_sysctl.c +6 -5
index 89921a0d2ebb..e9d4fc40d832 100644
@@ -464,7 +464,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 
 	inode = new_inode(sb);
 	if (!inode)
-		goto out;
+		return ERR_PTR(-ENOMEM);
 
 	inode->i_ino = get_next_ino();
 

@@ -474,7 +474,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	if (unlikely(head->unregistering)) {
 		spin_unlock(&sysctl_lock);
 		iput(inode);
-		inode = NULL;
+		inode = ERR_PTR(-ENOENT);
 		goto out;
 	}
 	ei->sysctl = head;

@@ -549,10 +549,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
 			goto out;
 	}
 
-	err = ERR_PTR(-ENOMEM);
 	inode = proc_sys_make_inode(dir->i_sb, h ? h : head, p);
-	if (!inode)
+	if (IS_ERR(inode)) {
+		err = ERR_CAST(inode);
 		goto out;
+	}
 
 	d_set_d_op(dentry, &proc_sys_dentry_operations);
 	err = d_splice_alias(inode, dentry);

@@ -685,7 +686,7 @@ static bool proc_sys_fill_cache(struct file *file,
 		if (d_in_lookup(child)) {
 			struct dentry *res;
 			inode = proc_sys_make_inode(dir->d_sb, head, table);
-			if (!inode) {
+			if (IS_ERR(inode)) {
 				d_lookup_done(child);
 				dput(child);
 				return false;
Signature · 1 line
2.20.0
maintainer Applied
#3 lore raw

fs/proc/proc_sysctl.c static struct inode *proc_sys_make_inode(struct super_block *sb, · line 479

Quoted 8 lines · Ivan Delalande
fs/proc/proc_sysctl.c +1 -1
@@ -474,7 +474,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	if (unlikely(head->unregistering)) {
 		spin_unlock(&sysctl_lock);
 		iput(inode);
-		inode = NULL;
+		inode = ERR_PTR(-ENOENT);
 		goto out;
 	}

Applied, with one modification: if you look at the target of that goto, you'll see
out:

        return inode;
so this place should be simply
 		spin_unlock(&sysctl_lock);
 		iput(inode);
		return ERR_PTR(-ENOENT);
	}

That way the label becomes unused and goes away.