xfs: Uninitialized memory read at xlog_write
diff mbox

Message ID 201709141915.HAD04123.FQOSHJtVOLFMFO@I-love.SAKURA.ne.jp
State New
Headers show

Commit Message

Tetsuo Handa Sept. 14, 2017, 10:15 a.m. UTC
Dave Chinner wrote:
> On Wed, Sep 13, 2017 at 06:59:38PM +0900, Tetsuo Handa wrote:
> > Dave Chinner wrote:
> > > On Wed, Sep 13, 2017 at 04:14:37PM +0900, Tetsuo Handa wrote:
> > > > [  OK  ] Stopped target Switch Root.
> > > > 
> > > > [  OK  ] Stopped target Initrd File Systems.[ 1054.691505] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff880135396660)
> > > > [ 1054.691506] 000000000000000093050a200000000000000000000000000000000000000000
> > > > [ 1054.691511]  u u u u u u u u i i i i i i i i u u u u u u u u u u u u u u u u
> > > > [ 1054.691515]  ^
> > > > [ 1054.691519] RIP: 0010:xlog_write+0x344/0x6b0
> > > 
> > > What line of code does this correspond to?
> > > 
> > 
> >                         /*
> >                          * Copy region.
> >                          *
> >                          * Unmount records just log an opheader, so can have
> >                          * empty payloads with no data region to copy. Hence we
> >                          * only copy the payload if the vector says it has data
> >                          * to copy.
> >                          */
> >                         ASSERT(copy_len >= 0);
> >                         if (copy_len > 0) {
> >                                 memcpy(ptr, reg->i_addr + copy_off, copy_len); // <= xlog_write+0x344/0x6b0
> >                                 xlog_write_adv_cnt(&ptr, &len, &log_offset,
> >                                                    copy_len);
> >                         }
> > 
> 
> Ok, that's what I suspected. The region being copied is set up
> in xlog_cil_insert_format_items(), so problem is in one of the
> ->iop_format methods it calls to format the dirty metadata into the
> region.
> 
> And given that the address is ...6660, it's likely the offset into
> the structure being copied is 96 bytes.
> 
> $ pahole...
> .....
> struct xfs_log_dinode {
> .....
>        xfs_agino_t                di_next_unlinked;     /*    96     4 */
> .....
> 
> Try the patch below.

That patch did not help.

I checked values passed to memcpy() using below patch.

----------
----------

The copy_len was not multiple of sizeof(struct xfs_log_dinode).
Thus, I guess we can't assume this is "struct xfs_log_dinode".

----------
         Starting Load/Save Random Seed...

         Starting Configure read-only root support...

[ 1106.927991] ptr=ffffc90001c08218 reg->i_addr=ffff880134c7fda8 copy_off=0 copy_len=16
[ 1106.928022] ptr=ffffc90001c08234 reg->i_addr=ffff88013395f858 copy_off=0 copy_len=56
[ 1106.928100] ptr=ffffc90001c08278 reg->i_addr=ffff88013395f890 copy_off=0 copy_len=96
[ 1106.932354] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff88013395f860)
[ 1106.932355] 58f895330188ffff0c0e06040000000000000000000000000000000000000000
[ 1106.932362]  u u u u u u u u i i i i i i i i u u u u u u u u u u u u u u u u
[ 1106.932368]  ^
[ 1106.932432] RIP: 0010:xlog_write+0x69a/0x730
[ 1106.932433] RSP: 0018:ffff880134c7fcc8 EFLAGS: 00010282
[ 1106.932434] RAX: 0000000000000038 RBX: ffff88013395f800 RCX: 000000000000000c
[ 1106.932434] RDX: ffffc90001c08234 RSI: ffff88013395f860 RDI: ffffc90001c0823c
[ 1106.932435] RBP: ffff880134c7fd70 R08: 0000000000000000 R09: 0000000000000000
[ 1106.932435] R10: ffffffff81dedfd8 R11: 0000000000000000 R12: 0000000000000038
[ 1106.932436] R13: 0000000000000002 R14: 0000000000000000 R15: ffff88013487f000
[ 1106.932437] FS:  0000000000000000(0000) GS:ffff88013f400000(0000) knlGS:0000000000000000
[ 1106.932438] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1106.932461] CR2: ffff88013486e418 CR3: 00000001339c9004 CR4: 00000000000606f0
[ 1106.932465]  xlog_write+0x69a/0x730
[ 1106.932467]  xlog_cil_push+0x240/0x460
[ 1106.932468]  xlog_cil_push_work+0x10/0x20
[ 1106.932470]  process_one_work+0x121/0x2a0
[ 1106.932471]  worker_thread+0x1b7/0x390
[ 1106.932472]  kthread+0xff/0x140
[ 1106.932506]  ret_from_fork+0x22/0x30
[ 1106.932509]  0xffffffffffffffff
[ 1137.332948] ptr=ffffc90001c10218 reg->i_addr=ffff880133bbbda8 copy_off=0 copy_len=16
[ 1137.332976] ptr=ffffc90001c10234 reg->i_addr=ffff880135240258 copy_off=0 copy_len=24
[ 1137.333024] ptr=ffffc90001c10258 reg->i_addr=ffff880135240270 copy_off=0 copy_len=384
[ 1167.850472] ptr=ffffc90001c18218 reg->i_addr=ffff88013182fda8 copy_off=0 copy_len=16
[ 1167.850503] ptr=ffffc90001c18234 reg->i_addr=ffff880136614a58 copy_off=0 copy_len=24
[ 1167.850555] ptr=ffffc90001c18258 reg->i_addr=ffff880136614a70 copy_off=0 copy_len=384
         Starting udev Coldplug all Devices...

         Starting Create Static Device Nodes in /dev...
----------

----------
[ 1561.441679] ptr=ffffc90001c08218 reg->i_addr=ffff880134c7fda8 copy_off=0 copy_len=16
[ 1561.441708] ptr=ffffc90001c08234 reg->i_addr=ffff8801319a4058 copy_off=0 copy_len=24
[ 1561.441755] ptr=ffffc90001c08258 reg->i_addr=ffff8801319a4070 copy_off=0 copy_len=128
[ 1561.441881] ptr=ffffc90001c082e4 reg->i_addr=ffff880131452068 copy_off=0 copy_len=24
[ 1561.441928] ptr=ffffc90001c08308 reg->i_addr=ffff880131452080 copy_off=0 copy_len=128
[ 1561.442048] ptr=ffffc90001c08394 reg->i_addr=ffff880131452100 copy_off=0 copy_len=3840
[ 1561.448086] ptr=ffffc90001c092a0 reg->i_addr=ffff88013636d068 copy_off=0 copy_len=24
[ 1561.448134] ptr=ffffc90001c092c4 reg->i_addr=ffff88013636d080 copy_off=0 copy_len=128
[ 1561.448253] ptr=ffffc90001c09350 reg->i_addr=ffff88013636d100 copy_off=0 copy_len=3456
[ 1561.454000] ptr=ffffc90001c0a0dc reg->i_addr=ffff880135b7a868 copy_off=0 copy_len=56
[ 1561.454073] ptr=ffffc90001c0a120 reg->i_addr=ffff880135b7a8a0 copy_off=0 copy_len=96
[ 1561.454170] ptr=ffffc90001c0a18c reg->i_addr=ffff880135b7a900 copy_off=0 copy_len=16
[ 1561.455971] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff880135b7a874)
[ 1561.455973] 10000000070000003b12030005000000616310003a504e50e8ed120800000000
[ 1561.455979]  i i i i i i i i i i i i i i i i u u i i u u u u i i i i i i i i
[ 1561.455984]                                          ^
[ 1561.455989] RIP: 0010:xlog_write+0x69a/0x730
[ 1561.455989] RSP: 0018:ffff880134c7fcc8 EFLAGS: 00010282
[ 1561.455990] RAX: 0000000000000038 RBX: ffff880135b7a800 RCX: 000000000000000b
[ 1561.455991] RDX: ffffc90001c0a0dc RSI: ffff880135b7a874 RDI: ffffc90001c0a0e8
[ 1561.455991] RBP: ffff880134c7fd70 R08: 0000000000000000 R09: 0000000000000004
[ 1561.455992] R10: ffffffff81df2a14 R11: 0000000000000000 R12: 0000000000000038
[ 1561.455992] R13: 000000000000000a R14: 0000000000000000 R15: ffff88013487f170
[ 1561.455993] FS:  0000000000000000(0000) GS:ffff88013f400000(0000) knlGS:0000000000000000
[ 1561.455994] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1561.455997] CR2: ffff88013486e418 CR3: 00000001319fc005 CR4: 00000000000606f0
[ 1561.455999]  xlog_write+0x69a/0x730
[ 1561.456000]  xlog_cil_push+0x240/0x460
[ 1561.456002]  xlog_cil_push_work+0x10/0x20
[ 1561.456003]  process_one_work+0x121/0x2a0
[ 1561.456004]  worker_thread+0x1b7/0x390
[ 1561.456005]  kthread+0xff/0x140
[ 1561.456007]  ret_from_fork+0x22/0x30
[ 1561.456009]  0xffffffffffffffff
----------
--
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

Comments

Darrick J. Wong Sept. 18, 2017, 6:21 p.m. UTC | #1
On Fri, Sep 15, 2017 at 08:44:31AM +1000, Dave Chinner wrote:
> On Thu, Sep 14, 2017 at 07:15:38PM +0900, Tetsuo Handa wrote:
> > Dave Chinner wrote:
> > > On Wed, Sep 13, 2017 at 06:59:38PM +0900, Tetsuo Handa wrote:
> > > > Dave Chinner wrote:
> > > > > On Wed, Sep 13, 2017 at 04:14:37PM +0900, Tetsuo Handa wrote:
> > > > > > [  OK  ] Stopped target Switch Root.
> > > > > > 
> > > > > > [  OK  ] Stopped target Initrd File Systems.[ 1054.691505] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff880135396660)
> > > > > > [ 1054.691506] 000000000000000093050a200000000000000000000000000000000000000000
> > > > > > [ 1054.691511]  u u u u u u u u i i i i i i i i u u u u u u u u u u u u u u u u
> > > > > > [ 1054.691515]  ^
> > > > > > [ 1054.691519] RIP: 0010:xlog_write+0x344/0x6b0
> > > > > 
> > > > > What line of code does this correspond to?
> > > > > 
> > > > 
> > > >                         /*
> > > >                          * Copy region.
> > > >                          *
> > > >                          * Unmount records just log an opheader, so can have
> > > >                          * empty payloads with no data region to copy. Hence we
> > > >                          * only copy the payload if the vector says it has data
> > > >                          * to copy.
> > > >                          */
> > > >                         ASSERT(copy_len >= 0);
> > > >                         if (copy_len > 0) {
> > > >                                 memcpy(ptr, reg->i_addr + copy_off, copy_len); // <= xlog_write+0x344/0x6b0
> > > >                                 xlog_write_adv_cnt(&ptr, &len, &log_offset,
> > > >                                                    copy_len);
> > > >                         }
> > > > 
> > > 
> > > Ok, that's what I suspected. The region being copied is set up
> > > in xlog_cil_insert_format_items(), so problem is in one of the
> > > ->iop_format methods it calls to format the dirty metadata into the
> > > region.
> > > 
> > > And given that the address is ...6660, it's likely the offset into
> > > the structure being copied is 96 bytes.
> > > 
> > > $ pahole...
> > > .....
> > > struct xfs_log_dinode {
> > > .....
> > >        xfs_agino_t                di_next_unlinked;     /*    96     4 */
> > > .....
> > > 
> > > Try the patch below.
> > 
> > That patch did not help.
> > 
> > I checked values passed to memcpy() using below patch.
> 
> ok....
> 
> > 
> > ----------
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index c5107c7..f91c4c7 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -2476,6 +2476,8 @@
> >  			 */
> >  			ASSERT(copy_len >= 0);
> >  			if (copy_len > 0) {
> > +				printk(KERN_INFO "ptr=%p reg->i_addr=%p copy_off=%u copy_len=%u\n",
> > +				       ptr, reg->i_addr, copy_off, copy_len);
> 
> You need to print out the reg->i_type here. Then we'll know exactly
> where it came from.
> 
> >  				memcpy(ptr, reg->i_addr + copy_off, copy_len);
> >  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
> >  						   copy_len);
> > ----------
> > 
> > The copy_len was not multiple of sizeof(struct xfs_log_dinode).
> > Thus, I guess we can't assume this is "struct xfs_log_dinode".
> 
> It still could be - we don't always log the entire structure.
> 
> static inline uint xfs_log_dinode_size(int version)
> {
>         if (version == 3)
> 		return sizeof(struct xfs_log_dinode);
> 	return offsetof(struct xfs_log_dinode, di_next_unlinked);
> }
> 
> i.e. If it's v2 inode, the logged region is 96 bytes in length, not
> 176.
> 
> > 
> > ----------
> >          Starting Load/Save Random Seed...
> > 
> >          Starting Configure read-only root support...
> > 
> > [ 1106.927991] ptr=ffffc90001c08218 reg->i_addr=ffff880134c7fda8 copy_off=0 copy_len=16
> > [ 1106.928022] ptr=ffffc90001c08234 reg->i_addr=ffff88013395f858 copy_off=0 copy_len=56
> > [ 1106.928100] ptr=ffffc90001c08278 reg->i_addr=ffff88013395f890 copy_off=0 copy_len=96
> > [ 1106.932354] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff88013395f860)
> 
> Hold on- that warning has come from the /prior/ region copy, not
> the current region!
> 
> i.e. 2nd last copy region was ffff88013395f858 for 0x38 bytes, and
> this spans address that failed ffff88013395f860. i.e. it's 8 bytes
> into that region. It's 48 bytes before the region that we were
> copying when kmemcheck triggered!
> 
> I'm going to guess that we've got a log op header, followed by
> a inode format header, follow by something like a v2 inode core.
> i.e. log op headers are 16 bytes, xfs_inode_log_format are 56 bytes,
> and a v2 inode core is 96 bytes.
> 
> So, the inode log format header:
> 
> struct xfs_inode_log_format {
>         uint16_t                   ilf_type;             /*     0     2 */
>         uint16_t                   ilf_size;             /*     2     2 */
>         uint32_t                   ilf_fields;           /*     4     4 */
>         uint16_t                   ilf_asize;            /*     8     2 */
>         uint16_t                   ilf_dsize;            /*    10     2 */
> 
>         /* XXX 4 bytes hole, try to pack */
> 
>         uint64_t                   ilf_ino;              /*    16     8 */
>         union {
>                 uint32_t           ilfu_rdev;            /*           4 */
>                 uuid_t             ilfu_uuid;            /*          16 */
>         } ilf_u;                                         /*    24    16 */
>         int64_t                    ilf_blkno;            /*    40     8 */
>         int32_t                    ilf_len;              /*    48     4 */
>         int32_t                    ilf_boffset;          /*    52     4 */
> 
>         /* size: 56, cachelines: 1, members: 10 */
>         /* sum members: 52, holes: 1, sum holes: 4 */
>         /* last cacheline: 56 bytes */
> };
> 
> Oh, that's right. Someone, a long, long time ago, screwed up the on
> disk inode log structure but nobody noticed it due to the Irix MIPS
> compilers padding the structure identically on 32 and 64 bit
> systems. Port to linux, and i386/x86-64 padded it differently. The
> fix at the time was to make it right in recovery and continue to
> use the native structure at runtime. Ok, let's fix that properly
> now.
> 
> Try the patch below.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> xfs: Don't log uninitialised fields in inode structures
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> Prevent kmemcheck from throwing warnings about reading uninitialised
> memory when formatting inodes into the incore log buffer. There are
> several issues here - we don't always log all the fields in the
> inode log format item, and we never log the inode the
> di_next_unlinked field.
> 
> In the case of the inode log format item, this is aused b cerbated

"is aused b cerbated" ?

> by the old xfs_inode_log_format structure padding issue. Hence make
> the padded, 64 bit aligned version of the structure the one we always
> use for formatting the log and get rid of the 64 bit variant. This
> means we'll always log the 64-bit version and so recovery only needs
> to convert from the unpadded 32 bit version from older 32 bit
> kernels.

And those old 32-bit kernels can read the xfs_inode_log_format{,_64}
structures, right?

> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_log_format.h | 27 +++++----------
>  fs/xfs/xfs_inode_item.c        | 79 ++++++++++++++++++++++--------------------
>  fs/xfs/xfs_ondisk.h            |  2 +-
>  3 files changed, 50 insertions(+), 58 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 8372e9bcd7b6..71de185735e0 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -270,6 +270,7 @@ typedef struct xfs_inode_log_format {
>  	uint32_t		ilf_fields;	/* flags for fields logged */
>  	uint16_t		ilf_asize;	/* size of attr d/ext/root */
>  	uint16_t		ilf_dsize;	/* size of data/ext/root */
> +	uint32_t		ilf_pad;	/* pad for 64 bit boundary */
>  	uint64_t		ilf_ino;	/* inode number */
>  	union {
>  		uint32_t	ilfu_rdev;	/* rdev value for dev inode*/
> @@ -280,29 +281,17 @@ typedef struct xfs_inode_log_format {
>  	int32_t			ilf_boffset;	/* off of inode in buffer */
>  } xfs_inode_log_format_t;
>  
> -typedef struct xfs_inode_log_format_32 {
> -	uint16_t		ilf_type;	/* inode log item type */
> -	uint16_t		ilf_size;	/* size of this item */
> -	uint32_t		ilf_fields;	/* flags for fields logged */
> -	uint16_t		ilf_asize;	/* size of attr d/ext/root */
> -	uint16_t		ilf_dsize;	/* size of data/ext/root */
> -	uint64_t		ilf_ino;	/* inode number */
> -	union {
> -		uint32_t	ilfu_rdev;	/* rdev value for dev inode*/
> -		uuid_t		ilfu_uuid;	/* mount point value */
> -	} ilf_u;
> -	int64_t			ilf_blkno;	/* blkno of inode buffer */
> -	int32_t			ilf_len;	/* len of inode buffer */
> -	int32_t			ilf_boffset;	/* off of inode in buffer */
> -} __attribute__((packed)) xfs_inode_log_format_32_t;
> -
> -typedef struct xfs_inode_log_format_64 {
> +/*
> + * Old 32 bit systems will log in this format without the 64 bit
> + * alignment padding. Recovery will detect this and convert it to the
> + * correct format.
> + */
> +struct xfs_inode_log_format_32 {
>  	uint16_t		ilf_type;	/* inode log item type */
>  	uint16_t		ilf_size;	/* size of this item */
>  	uint32_t		ilf_fields;	/* flags for fields logged */
>  	uint16_t		ilf_asize;	/* size of attr d/ext/root */
>  	uint16_t		ilf_dsize;	/* size of data/ext/root */
> -	uint32_t		ilf_pad;	/* pad for 64 bit boundary */
>  	uint64_t		ilf_ino;	/* inode number */
>  	union {
>  		uint32_t	ilfu_rdev;	/* rdev value for dev inode*/
> @@ -311,7 +300,7 @@ typedef struct xfs_inode_log_format_64 {
>  	int64_t			ilf_blkno;	/* blkno of inode buffer */
>  	int32_t			ilf_len;	/* len of inode buffer */
>  	int32_t			ilf_boffset;	/* off of inode in buffer */
> -} xfs_inode_log_format_64_t;
> +} __attribute__((packed));
>  
>  
>  /*
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 6d0f74ec31e8..aec9cf36b5b7 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -364,6 +364,9 @@ xfs_inode_to_log_dinode(
>  	to->di_dmstate = from->di_dmstate;
>  	to->di_flags = from->di_flags;
>  
> +	/* log a dummy value to ensure log structure is fully initialised */
> +	to->di_next_unlinked = NULLAGINO;
> +
>  	if (from->di_version == 3) {
>  		to->di_changecount = inode->i_version;
>  		to->di_crtime.t_sec = from->di_crtime.t_sec;
> @@ -404,6 +407,11 @@ xfs_inode_item_format_core(
>   * the second with the on-disk inode structure, and a possible third and/or
>   * fourth with the inode data/extents/b-tree root and inode attributes
>   * data/extents/b-tree root.
> + *
> + * Note: Always use the 64 bit inode log format structure so we don't
> + * leave an uninitialised hole in the format item on 64 bit systems. Log
> + * recovery on 32 bit systems handles this just fine, so there's no reason
> + * for not using an initialising the properly padded structure all the time.
>   */
>  STATIC void
>  xfs_inode_item_format(
> @@ -412,8 +420,8 @@ xfs_inode_item_format(
>  {
>  	struct xfs_inode_log_item *iip = INODE_ITEM(lip);
>  	struct xfs_inode	*ip = iip->ili_inode;
> -	struct xfs_inode_log_format *ilf;
>  	struct xfs_log_iovec	*vecp = NULL;
> +	struct xfs_inode_log_format *ilf;
>  
>  	ASSERT(ip->i_d.di_version > 1);
>  
> @@ -425,7 +433,17 @@ xfs_inode_item_format(
>  	ilf->ilf_boffset = ip->i_imap.im_boffset;
>  	ilf->ilf_fields = XFS_ILOG_CORE;
>  	ilf->ilf_size = 2; /* format + core */
> -	xlog_finish_iovec(lv, vecp, sizeof(struct xfs_inode_log_format));
> +
> +	/*
> +	 * make sure we don't leak uninitialised data into the log in the case
> +	 * when we don't log every field in the inode.
> +	 */
> +	ilf->ilf_dsize = 0;
> +	ilf->ilf_asize = 0;
> +	ilf->ilf_pad = 0;
> +	uuid_copy(&ilf->ilf_u.ilfu_uuid, &uuid_null);
> +
> +	xlog_finish_iovec(lv, vecp, sizeof(*ilf));
>  
>  	xfs_inode_item_format_core(ip, lv, &vecp);
>  	xfs_inode_item_format_data_fork(iip, ilf, lv, &vecp);
> @@ -855,44 +873,29 @@ xfs_istale_done(
>  }
>  
>  /*
> - * convert an xfs_inode_log_format struct from either 32 or 64 bit versions
> - * (which can have different field alignments) to the native version
> + * convert an xfs_inode_log_format struct from the old 32 bit version
> + * (which can have different field alignments) to the native 64 bit version
>   */
>  int
>  xfs_inode_item_format_convert(
> -	xfs_log_iovec_t		*buf,
> -	xfs_inode_log_format_t	*in_f)
> +	struct xfs_log_iovec		*buf,
> +	struct xfs_inode_log_format	*in_f)
>  {
> -	if (buf->i_len == sizeof(xfs_inode_log_format_32_t)) {
> -		xfs_inode_log_format_32_t *in_f32 = buf->i_addr;
> -
> -		in_f->ilf_type = in_f32->ilf_type;
> -		in_f->ilf_size = in_f32->ilf_size;
> -		in_f->ilf_fields = in_f32->ilf_fields;
> -		in_f->ilf_asize = in_f32->ilf_asize;
> -		in_f->ilf_dsize = in_f32->ilf_dsize;
> -		in_f->ilf_ino = in_f32->ilf_ino;
> -		/* copy biggest field of ilf_u */
> -		uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f32->ilf_u.ilfu_uuid);
> -		in_f->ilf_blkno = in_f32->ilf_blkno;
> -		in_f->ilf_len = in_f32->ilf_len;
> -		in_f->ilf_boffset = in_f32->ilf_boffset;
> -		return 0;
> -	} else if (buf->i_len == sizeof(xfs_inode_log_format_64_t)){
> -		xfs_inode_log_format_64_t *in_f64 = buf->i_addr;
> -
> -		in_f->ilf_type = in_f64->ilf_type;
> -		in_f->ilf_size = in_f64->ilf_size;
> -		in_f->ilf_fields = in_f64->ilf_fields;
> -		in_f->ilf_asize = in_f64->ilf_asize;
> -		in_f->ilf_dsize = in_f64->ilf_dsize;
> -		in_f->ilf_ino = in_f64->ilf_ino;
> -		/* copy biggest field of ilf_u */
> -		uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f64->ilf_u.ilfu_uuid);
> -		in_f->ilf_blkno = in_f64->ilf_blkno;
> -		in_f->ilf_len = in_f64->ilf_len;
> -		in_f->ilf_boffset = in_f64->ilf_boffset;
> -		return 0;
> -	}
> -	return -EFSCORRUPTED;
> +	struct xfs_inode_log_format_32	*in_f32 = buf->i_addr;
> +
> +	if (buf->i_len != sizeof(*in_f32))
> +		return -EFSCORRUPTED;
> +
> +	in_f->ilf_type = in_f32->ilf_type;
> +	in_f->ilf_size = in_f32->ilf_size;
> +	in_f->ilf_fields = in_f32->ilf_fields;
> +	in_f->ilf_asize = in_f32->ilf_asize;
> +	in_f->ilf_dsize = in_f32->ilf_dsize;
> +	in_f->ilf_ino = in_f32->ilf_ino;
> +	/* copy biggest field of ilf_u */
> +	uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f32->ilf_u.ilfu_uuid);
> +	in_f->ilf_blkno = in_f32->ilf_blkno;
> +	in_f->ilf_len = in_f32->ilf_len;
> +	in_f->ilf_boffset = in_f32->ilf_boffset;
> +	return 0;
>  }
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 0c381d71b242..0492436a053f 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -134,7 +134,7 @@ xfs_check_ondisk_structs(void)
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_icreate_log,		28);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_ictimestamp,		8);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format_32,	52);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format_64,	56);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format,	56);

Will require a change to xfs/122 as well...

--D

>  	XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat,	20);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
>  }
> --
> 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
Dave Chinner Sept. 20, 2017, 12:46 a.m. UTC | #2
On Mon, Sep 18, 2017 at 11:21:21AM -0700, Darrick J. Wong wrote:
> On Fri, Sep 15, 2017 at 08:44:31AM +1000, Dave Chinner wrote:
> > xfs: Don't log uninitialised fields in inode structures
> > 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Prevent kmemcheck from throwing warnings about reading uninitialised
> > memory when formatting inodes into the incore log buffer. There are
> > several issues here - we don't always log all the fields in the
> > inode log format item, and we never log the inode the
> > di_next_unlinked field.
> > 
> > In the case of the inode log format item, this is aused b cerbated
> 
> "is aused b cerbated" ?

vi fail when replacing the word "used" with "exacerbated". :)

> > by the old xfs_inode_log_format structure padding issue. Hence make
> > the padded, 64 bit aligned version of the structure the one we always
> > use for formatting the log and get rid of the 64 bit variant. This
> > means we'll always log the 64-bit version and so recovery only needs
> > to convert from the unpadded 32 bit version from older 32 bit
> > kernels.
> 
> And those old 32-bit kernels can read the xfs_inode_log_format{,_64}
> structures, right?

Yes. With the in-memory format now being the same as the old
xfs_inode_log_format_64 structure in the log, we only need a
conversion from "32 bit in log to 64 bit in memory" on all
platforms.

Cheers,

Dave.
Darrick J. Wong Sept. 20, 2017, 12:49 a.m. UTC | #3
On Wed, Sep 20, 2017 at 10:46:39AM +1000, Dave Chinner wrote:
> On Mon, Sep 18, 2017 at 11:21:21AM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 15, 2017 at 08:44:31AM +1000, Dave Chinner wrote:
> > > xfs: Don't log uninitialised fields in inode structures
> > > 
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Prevent kmemcheck from throwing warnings about reading uninitialised
> > > memory when formatting inodes into the incore log buffer. There are
> > > several issues here - we don't always log all the fields in the
> > > inode log format item, and we never log the inode the
> > > di_next_unlinked field.
> > > 
> > > In the case of the inode log format item, this is aused b cerbated
> > 
> > "is aused b cerbated" ?
> 
> vi fail when replacing the word "used" with "exacerbated". :)
> 
> > > by the old xfs_inode_log_format structure padding issue. Hence make
> > > the padded, 64 bit aligned version of the structure the one we always
> > > use for formatting the log and get rid of the 64 bit variant. This
> > > means we'll always log the 64-bit version and so recovery only needs
> > > to convert from the unpadded 32 bit version from older 32 bit
> > > kernels.
> > 
> > And those old 32-bit kernels can read the xfs_inode_log_format{,_64}
> > structures, right?
> 
> Yes. With the in-memory format now being the same as the old
> xfs_inode_log_format_64 structure in the log, we only need a
> conversion from "32 bit in log to 64 bit in memory" on all
> platforms.

Ok, that's what I thought.  Will await a positive test result + proper
patch submission. :)

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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

Patch
diff mbox

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index c5107c7..f91c4c7 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2476,6 +2476,8 @@ 
 			 */
 			ASSERT(copy_len >= 0);
 			if (copy_len > 0) {
+				printk(KERN_INFO "ptr=%p reg->i_addr=%p copy_off=%u copy_len=%u\n",
+				       ptr, reg->i_addr, copy_off, copy_len);
 				memcpy(ptr, reg->i_addr + copy_off, copy_len);
 				xlog_write_adv_cnt(&ptr, &len, &log_offset,
 						   copy_len);