[bug report] xfs: scrub inodes

[bug report] xfs: scrub inodes

[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);
                                                  ^^
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

Re: [bug report] xfs: scrub inodes

[Darrick J. Wong]

On Sat, Nov 04, 2017 at 11:02:43AM +0300, Dan Carpenter wrote:
> 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...

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

>    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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html