Skip to contentSkip to conversation

xfs: scrub inodes

bug report Dan Carpenter 2 participants 2 messages Nov 4, 2017 – Nov 6, 2017

lore
Share & tools
lore
#0 lore raw

Hello Darrick J. Wong,

This is a semi-automatic email about new static checker warnings.

The patch 80e4e1268802: "xfs: scrub inodes" from Oct 17, 2017, leads to the following Smatch complaint:

fs/xfs/scrub/inode.c:356 xfs_scrub_dinode()

	 error: we previously assumed 'sc->ip' could be null (see line 338)

fs/xfs/scrub/inode.c
337
338 if (dip->di_mode == 0 && sc->ip)

                                                 ^^^^^^

The patch adds a check if sc->ip is NULL

339 xfs_scrub_ino_set_corrupt(sc, ino, bp);
340
341 if (dip->di_projid_hi != 0 &&
342 !xfs_sb_version_hasprojid32bit(&mp->m_sb))
343 xfs_scrub_ino_set_corrupt(sc, ino, bp);
344 break;
345 default:
346 xfs_scrub_ino_set_corrupt(sc, ino, bp);
347 return;
348 }
349
350 /*
351 * di_uid/di_gid -- -1 isn't invalid, but there's no way that
352 * userspace could have created that.
353 */
354 if (dip->di_uid == cpu_to_be32(-1U) ||
355 dip->di_gid == cpu_to_be32(-1U))

   356			xfs_scrub_ino_set_warning(sc, bp);
                                                  ^^

But later we pass it to xfs_scrub_ino_set_warning() and it gets dereferenced without checking... I don't know the rules about sc->ip well enough to say when it's NULL or not...

357
358 /* di_format */

regards,
dan carpenter

#1 lore raw
Quoted 35 lines · Dan Carpenter

Hello Darrick J. Wong,

This is a semi-automatic email about new static checker warnings.

The patch 80e4e1268802: "xfs: scrub inodes" from Oct 17, 2017, leads to the following Smatch complaint:

fs/xfs/scrub/inode.c:356 xfs_scrub_dinode()

	 error: we previously assumed 'sc->ip' could be null (see line 338)

fs/xfs/scrub/inode.c
337
338 if (dip->di_mode == 0 && sc->ip)

                                                 ^^^^^^

The patch adds a check if sc->ip is NULL

   339				xfs_scrub_ino_set_corrupt(sc, ino, bp);
   340	
   341			if (dip->di_projid_hi != 0 &&
   342			    !xfs_sb_version_hasprojid32bit(&mp->m_sb))
   343				xfs_scrub_ino_set_corrupt(sc, ino, bp);
   344			break;
   345		default:
   346			xfs_scrub_ino_set_corrupt(sc, ino, bp);
   347			return;
   348		}
   349	
   350		/*
   351		 * di_uid/di_gid -- -1 isn't invalid, but there's no way that
   352		 * userspace could have created that.
   353		 */
   354		if (dip->di_uid == cpu_to_be32(-1U) ||
   355		    dip->di_gid == cpu_to_be32(-1U))
   356			xfs_scrub_ino_set_warning(sc, bp);
                                                  ^^
Quoted 3 lines

But later we pass it to xfs_scrub_ino_set_warning() and it gets dereferenced without checking... I don't know the rules about sc->ip well enough to say when it's NULL or not...

xfs_scrub_ino_set_warning and xfs_scrub_ino_set_preen both need to take the inode number as a parameter and not rely on sc->ip pointing anywhere. I'll send a fix shortly; thank you for bringing this to my attention.

--D

Quoted 9 lines
   357	
   358		/* di_format */

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html