[PATCH] xfs: Fix xfs_attr_leafblock definition

[PATCH] xfs: Fix xfs_attr_leafblock definition

[Jan Kara]

struct xfs_attr_leafblock contains 'entries' array which is declared
with size 1 altough it can in fact contain much more entries. Since this
array is followed by further struct members, gcc (at least in version
4.8.3) thinks that the array has the fixed size of 1 element and thus
may optimize away all accesses beyond the end of array resulting in
non-working code. This problem was only observed with userspace code in
xfsprogs, however it's better to be safe in kernel as well and have
matching kernel and xfsprogs definitions.

Signed-off-by: Jan Kara <jack@suse.com>
---
 fs/xfs/libxfs/xfs_da_format.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

 This is a kernel version of the xfsprogs patch I've sent a while ago.

diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 74bcbabfa523..b14bbd6bb05f 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -680,8 +680,15 @@ typedef struct xfs_attr_leaf_name_remote {
 typedef struct xfs_attr_leafblock {
 	xfs_attr_leaf_hdr_t	hdr;	/* constant-structure header block */
 	xfs_attr_leaf_entry_t	entries[1];	/* sorted on key, not name */
-	xfs_attr_leaf_name_local_t namelist;	/* grows from bottom of buf */
-	xfs_attr_leaf_name_remote_t valuelist;	/* grows from bottom of buf */
+	/*
+	 * The rest of the block contains the following structures after the
+	 * leaf entries, growing from the bottom up. The variables are never
+	 * referenced and definining them can actually make gcc optimize away
+	 * accesses to the 'entries' array above index 0 so don't do that.
+	 *
+	 * xfs_attr_leaf_name_local_t namelist;
+	 * xfs_attr_leaf_name_remote_t valuelist;
+	 */
 } xfs_attr_leafblock_t;
 
 /*
-- 
2.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix xfs_attr_leafblock definition

[Mark Tinguely]

On 08/13/15 04:26, Jan Kara wrote:
> struct xfs_attr_leafblock contains 'entries' array which is declared
> with size 1 altough it can in fact contain much more entries. Since this
> array is followed by further struct members, gcc (at least in version
> 4.8.3) thinks that the array has the fixed size of 1 element and thus
> may optimize away all accesses beyond the end of array resulting in
> non-working code. This problem was only observed with userspace code in
> xfsprogs, however it's better to be safe in kernel as well and have
> matching kernel and xfsprogs definitions.
>
> Signed-off-by: Jan Kara<jack@suse.com>
> ---

I hit this bug in SLES12 and RHEL7 XFS kernel code. It is the gcc 4.8's 
new loop optimization routine. A person can bypass it with the option:
     -fno-aggressive-loop-optimizations
but this fixes the source of the problem.

Acked-by: Mark Tinguely <tinguely@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix xfs_attr_leafblock definition

[Dave Chinner]

On Thu, Aug 13, 2015 at 08:17:17AM -0500, Mark Tinguely wrote:
> On 08/13/15 04:26, Jan Kara wrote:
> >struct xfs_attr_leafblock contains 'entries' array which is declared
> >with size 1 altough it can in fact contain much more entries. Since this
> >array is followed by further struct members, gcc (at least in version
> >4.8.3) thinks that the array has the fixed size of 1 element and thus
> >may optimize away all accesses beyond the end of array resulting in
> >non-working code. This problem was only observed with userspace code in
> >xfsprogs, however it's better to be safe in kernel as well and have
> >matching kernel and xfsprogs definitions.
> >
> >Signed-off-by: Jan Kara<jack@suse.com>
> >---
> 
> I hit this bug in SLES12 and RHEL7 XFS kernel code. It is the gcc
> 4.8's new loop optimization routine. A person can bypass it with the
> option:
>     -fno-aggressive-loop-optimizations
> but this fixes the source of the problem.
> 
> Acked-by: Mark Tinguely <tinguely@sgi.com>

Hi Mark - it's great that you had found and identified the exact
compiler optimisation that was causing miscompilation of the XFS
code.

Troy, what's not great about this is that SGI hasn't seen fit to
report this bug upstream or to their distro partners in a prompt
manner. Can you please make sure that any other XFS bug fixes that
SGI are sitting on get communicated to the appropriate venue so
they can be addressed without undue delay?

Thanks,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix xfs_attr_leafblock definition

[Eric Sandeen]

On 8/13/15 4:26 AM, Jan Kara wrote:
> struct xfs_attr_leafblock contains 'entries' array which is declared
> with size 1 altough it can in fact contain much more entries. Since this
> array is followed by further struct members, gcc (at least in version
> 4.8.3) thinks that the array has the fixed size of 1 element and thus
> may optimize away all accesses beyond the end of array resulting in
> non-working code. This problem was only observed with userspace code in
> xfsprogs, however it's better to be safe in kernel as well and have
> matching kernel and xfsprogs definitions.
> 
> Signed-off-by: Jan Kara <jack@suse.com>
> ---
>  fs/xfs/libxfs/xfs_da_format.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
>  This is a kernel version of the xfsprogs patch I've sent a while ago.
> 
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 74bcbabfa523..b14bbd6bb05f 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -680,8 +680,15 @@ typedef struct xfs_attr_leaf_name_remote {
>  typedef struct xfs_attr_leafblock {
>  	xfs_attr_leaf_hdr_t	hdr;	/* constant-structure header block */
>  	xfs_attr_leaf_entry_t	entries[1];	/* sorted on key, not name */
> -	xfs_attr_leaf_name_local_t namelist;	/* grows from bottom of buf */
> -	xfs_attr_leaf_name_remote_t valuelist;	/* grows from bottom of buf */
> +	/*
> +	 * The rest of the block contains the following structures after the
> +	 * leaf entries, growing from the bottom up. The variables are never
> +	 * referenced and definining them can actually make gcc optimize away

Nitpick, s/definining/defining/, maybe can be fixed on commit :)

Don't we have other similar constructs elsewhere?  Do they suffer the same
fate?

... looks ...

I guess not; the other ones have the variable length array as the last member.

-Eric

> +	 * accesses to the 'entries' array above index 0 so don't do that.
> +	 *
> +	 * xfs_attr_leaf_name_local_t namelist;
> +	 * xfs_attr_leaf_name_remote_t valuelist;
> +	 */
>  } xfs_attr_leafblock_t;
>  
>  /*
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs