| Message ID | 1519134782-10062-1-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 20, 2018 at 03:53:02PM +0200, Nikolay Borisov wrote: > The check performed before the memcpy responsible for copying the rest > of the inode is already performed before we call xfs_log_dinode_to_disk. > So let's remove the 2nd instance of the check. No functional changes. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > Reported-by: Jeff Mahoney <jeffm@suse.com> Looks ok, will test... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/xfs_log_recover.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 00240c9ee72e..88dccfb1de96 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2( > xfs_log_dinode_to_disk(ldip, dip); > > /* the rest is in on-disk format */ > - if (item->ri_buf[1].i_len > isize) { > - memcpy((char *)dip + isize, > - item->ri_buf[1].i_addr + isize, > + memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize, > item->ri_buf[1].i_len - isize); > - } > > fields = in_f->ilf_fields; > if (fields & XFS_ILOG_DEV) > -- > 2.7.4 > > -- > 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 -- 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
On Tue, Feb 20, 2018 at 03:53:02PM +0200, Nikolay Borisov wrote: > The check performed before the memcpy responsible for copying the rest > of the inode is already performed before we call xfs_log_dinode_to_disk. > So let's remove the 2nd instance of the check. No functional changes. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > Reported-by: Jeff Mahoney <jeffm@suse.com> > --- > fs/xfs/xfs_log_recover.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 00240c9ee72e..88dccfb1de96 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2( > xfs_log_dinode_to_disk(ldip, dip); > > /* the rest is in on-disk format */ > - if (item->ri_buf[1].i_len > isize) { > - memcpy((char *)dip + isize, > - item->ri_buf[1].i_addr + isize, > + memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize, > item->ri_buf[1].i_len - isize); > - } > This looks wrong. The previous check is: if (unlikely(item->ri_buf[1].i_len > isize)) { CORRUPTION_ERROR .... error = -EFSCORRUPTED; goto out_release; } So after this item->ri_buf[1].i_len is always <= isize. IOWs, the memcpy() is currently dead code that is never executed, not code we want to execute in every inode recovery. Cheers, Dave.
On 20.02.2018 23:11, Dave Chinner wrote: > On Tue, Feb 20, 2018 at 03:53:02PM +0200, Nikolay Borisov wrote: >> The check performed before the memcpy responsible for copying the rest >> of the inode is already performed before we call xfs_log_dinode_to_disk. >> So let's remove the 2nd instance of the check. No functional changes. >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> Reported-by: Jeff Mahoney <jeffm@suse.com> >> --- >> fs/xfs/xfs_log_recover.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c >> index 00240c9ee72e..88dccfb1de96 100644 >> --- a/fs/xfs/xfs_log_recover.c >> +++ b/fs/xfs/xfs_log_recover.c >> @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2( >> xfs_log_dinode_to_disk(ldip, dip); >> >> /* the rest is in on-disk format */ >> - if (item->ri_buf[1].i_len > isize) { >> - memcpy((char *)dip + isize, >> - item->ri_buf[1].i_addr + isize, >> + memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize, >> item->ri_buf[1].i_len - isize); >> - } >> > > This looks wrong. > > The previous check is: > > if (unlikely(item->ri_buf[1].i_len > isize)) { > CORRUPTION_ERROR > .... > error = -EFSCORRUPTED; > goto out_release; > } > > So after this item->ri_buf[1].i_len is always <= isize. IOWs, the > memcpy() is currently dead code that is never executed, not code we > want to execute in every inode recovery. Doh, you are right, will resend. > > Cheers, > > Dave. > -- 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
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 00240c9ee72e..88dccfb1de96 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2( xfs_log_dinode_to_disk(ldip, dip); /* the rest is in on-disk format */ - if (item->ri_buf[1].i_len > isize) { - memcpy((char *)dip + isize, - item->ri_buf[1].i_addr + isize, + memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize, item->ri_buf[1].i_len - isize); - } fields = in_f->ilf_fields; if (fields & XFS_ILOG_DEV)
The check performed before the memcpy responsible for copying the rest of the inode is already performed before we call xfs_log_dinode_to_disk. So let's remove the 2nd instance of the check. No functional changes. Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reported-by: Jeff Mahoney <jeffm@suse.com> --- fs/xfs/xfs_log_recover.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)