Skip to contentSkip to conversation

xfs: Fix error pointer dereference

PATCH Superseded Ethan Tidmore 2 participants 2 messages Feb 19

Subsystems (2)
Stable branch XFS filesystem

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Ethan Tidmore

author
#0 lore raw

PATCH v2 1/1 xfs: Fix error pointer dereference

The function try_lookup_noperm() can return an error pointer and is not checked for one. Add checks for error pointer and propagate it.

Fixes 73597e3e42b4 ("xfs: ensure dentry consistency when the orphanage adopts a file")
Cc <[email protected]> # v6.16
Signed-off-by Ethan Tidmore <[email protected]>
---
 fs/xfs/scrub/orphanage.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
fs/xfs/scrub/orphanage.c +14 -3
index 52a108f6d5f4..3269a0646e19 100644
@@ -442,6 +442,9 @@ xrep_adoption_check_dcache(
 		return 0;
 
 	d_child = try_lookup_noperm(&qname, d_orphanage);
+	if (IS_ERR(d_child))
+		return PTR_ERR(d_child);
+
 	if (d_child) {
 		trace_xrep_adoption_check_child(sc->mp, d_child);
 

@@ -464,7 +467,7 @@ xrep_adoption_check_dcache(
  * There should not be any positive entries for the name, since we've
  * maintained our lock on the orphanage directory.
  */
-static void
+static int
 xrep_adoption_zap_dcache(
 	struct xrep_adoption	*adopt)
 {

@@ -476,9 +479,12 @@ xrep_adoption_zap_dcache(
 	/* Invalidate all dentries for the adoption name */
 	d_orphanage = d_find_alias(VFS_I(sc->orphanage));
 	if (!d_orphanage)
-		return;
+		return 0;
 
 	d_child = try_lookup_noperm(&qname, d_orphanage);
+	if (IS_ERR(d_child))
+		return PTR_ERR(d_child);
+
 	while (d_child != NULL) {
 		trace_xrep_adoption_invalidate_child(sc->mp, d_child);
 

@@ -497,6 +503,8 @@ xrep_adoption_zap_dcache(
 		d_invalidate(d_child);
 		dput(d_child);
 	}
+
+	return 0;
 }
 
 /*

@@ -592,7 +600,10 @@ xrep_adoption_move(
 	xfs_dir_update_hook(sc->orphanage, sc->ip, 1, adopt->xname);
 
 	/* Remove negative dentries from the lost+found's dcache */
-	xrep_adoption_zap_dcache(adopt);
+	error = xrep_adoption_zap_dcache(adopt);
+	if (error)
+		return error;
+
 	return 0;
 }
Signature · 1 line
2.53.0
#1 lore raw
Quoted 15 lines · Ethan Tidmore

The function try_lookup_noperm() can return an error pointer and is not checked for one. Add checks for error pointer and propagate it.

Fixes 73597e3e42b4 ("xfs: ensure dentry consistency when the orphanage adopts a file")
Cc <[email protected]> # v6.16
Signed-off-by Ethan Tidmore <[email protected]>
---
 fs/xfs/scrub/orphanage.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
fs/xfs/scrub/orphanage.c +0 -0
index 52a108f6d5f4..3269a0646e19 100644

fs/xfs/scrub/orphanage.c xrep_adoption_check_dcache( · line 446

Quoted 5 lines
fs/xfs/scrub/orphanage.c +2 -0
@@ -442,6 +442,9 @@ xrep_adoption_check_dcache(
 		return 0;
 
 	d_child = try_lookup_noperm(&qname, d_orphanage);
+	if (IS_ERR(d_child))
+		return PTR_ERR(d_child);

I think you still need to dput(d_orphanage) below if you're returning an error code.

Quoted 13 lines
fs/xfs/scrub/orphanage.c +1 -1
 	if (d_child) {
 		trace_xrep_adoption_check_child(sc->mp, d_child);
 

@@ -464,7 +467,7 @@ xrep_adoption_check_dcache(
  * There should not be any positive entries for the name, since we've
  * maintained our lock on the orphanage directory.
  */
-static void
+static int
 xrep_adoption_zap_dcache(
 	struct xrep_adoption	*adopt)
 {

fs/xfs/scrub/orphanage.c xrep_adoption_zap_dcache( · line 486

Quoted 9 lines
fs/xfs/scrub/orphanage.c +3 -1
@@ -476,9 +479,12 @@ xrep_adoption_zap_dcache(
 	/* Invalidate all dentries for the adoption name */
 	d_orphanage = d_find_alias(VFS_I(sc->orphanage));
 	if (!d_orphanage)
-		return;
+		return 0;
 
 	d_child = try_lookup_noperm(&qname, d_orphanage);
+	if (IS_ERR(d_child))
+		return PTR_ERR(d_child);

Don't we still want to perform the d_find_alias loop below?

The changes you made to xrep_adoption_zap_dcache in the first version seemed reasonable to me since it only tried to invalidate a bunch of dcache entries. Also I think if this call to try_lookup_noperm() returns an ERR_PTR, then the call in _check_dcache would have gotten the same ERR_PTR return value and ended the entire repair attempt.

--D

Quoted 28 lines
fs/xfs/scrub/orphanage.c +7 -1
+
 	while (d_child != NULL) {
 		trace_xrep_adoption_invalidate_child(sc->mp, d_child);
 

@@ -497,6 +503,8 @@ xrep_adoption_zap_dcache(
 		d_invalidate(d_child);
 		dput(d_child);
 	}
+
+	return 0;
 }
 
 /*

@@ -592,7 +600,10 @@ xrep_adoption_move(
 	xfs_dir_update_hook(sc->orphanage, sc->ip, 1, adopt->xname);
 
 	/* Remove negative dentries from the lost+found's dcache */
-	xrep_adoption_zap_dcache(adopt);
+	error = xrep_adoption_zap_dcache(adopt);
+	if (error)
+		return error;
+
 	return 0;
 }
Signature · 1 line
2.53.0