[PATCH] xfs: fix in the setting of logbsize

[PATCH] xfs: fix in the setting of logbsize

[Ales Novak]

Logbsize should be integer multiple of log stripe unit size. If it's
not, various operations will lead to crashes, due to invalid buffer
sizes, i.e. we've seen crashes in the callpath xlog_sync->xlog_pack_data.

However, this rule is only mentioned in the documentation, while it
could be checked during the mount.

Signed-off-by: Ales Novak <alnovak@suse.cz>
---
 fs/xfs/xfs_super.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 8fcc4cc..1a3766d 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1352,6 +1352,11 @@ xfs_finish_flags(
 			xfs_warn(mp,
 		"logbuf size must be greater than or equal to log stripe size");
 			return -EINVAL;
+		} else if (mp->m_logbsize > 0 && mp->m_sb.sb_logsunit > 0 &&
+			   mp->m_logbsize % mp->m_sb.sb_logsunit) {
+			xfs_warn(mp,
+		"logbuf size must be integer multiple of log stripe size");
+			return -EINVAL;
 		}
 	} else {
 		/* Fail a mount if the logbuf is larger than 32K */
-- 
1.8.4.5

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Dave Chinner]

[dropped lkml from the cc list, XFS specific noise is not needed
there.]

On Fri, Jun 05, 2015 at 03:28:45PM +0200, Ales Novak wrote:
> Logbsize should be integer multiple of log stripe unit size. If it's
> not, various operations will lead to crashes, due to invalid buffer
> sizes, i.e. we've seen crashes in the callpath xlog_sync->xlog_pack_data.

Can you give more information about the crashes? From this
description, I do not know whether this is a work around or a fix
because I don't know  what code is actually causing the problem or
the circumstances in which the crash occurs. Hence I cannot
determine if your change is the right change to make or whether the
documetnation is simply wrong and we have a real bug we shoul dbe
fixing.

> However, this rule is only mentioned in the documentation, while it
> could be checked during the mount.

Where in the documentation is that mentioned?

> Signed-off-by: Ales Novak <alnovak@suse.cz>
> ---
>  fs/xfs/xfs_super.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 8fcc4cc..1a3766d 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1352,6 +1352,11 @@ xfs_finish_flags(
>  			xfs_warn(mp,
>  		"logbuf size must be greater than or equal to log stripe size");
>  			return -EINVAL;
> +		} else if (mp->m_logbsize > 0 && mp->m_sb.sb_logsunit > 0 &&
> +			   mp->m_logbsize % mp->m_sb.sb_logsunit) {
> +			xfs_warn(mp,
> +		"logbuf size must be integer multiple of log stripe size");
> +			return -EINVAL;
>  		}
>  	} else {
>  		/* Fail a mount if the logbuf is larger than 32K */

If the logbsize was not specified as a mount option, we simply use
what is on disk. If it was specified as a mount option, we know that
we've already constrained m_logbsize to 32k, 64k, 128k or 256k (in
xfs_parse_args()). Which then means the above code will refuse to
mount a filesystem where the logsunit is not a power of two. e.g.
logsunit of 96k and logbsize=192k will not be allowed to mount...

So, at minimum, we need to refine more than just check for integer
multiples here, and so we need to look at whether arbitrary stripe
unit alignments in mkfs are valid given the power-of-2 restriction
on the log buffer size which may be overly restrictive...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Eric Sandeen]

On 6/5/15 5:22 PM, Dave Chinner wrote:

> On Fri, Jun 05, 2015 at 03:28:45PM +0200, Ales Novak wrote:
>> However, this rule is only mentioned in the documentation, while it
>> could be checked during the mount.
> 
> Where in the documentation is that mentioned?

Documentation/filesystems/xfs.txt:

  logbsize=value
        Set the size of each in-memory log buffer.  The size may be
        specified in bytes, or in kilobytes with a "k" suffix.
        Valid sizes for version 1 and version 2 logs are 16384 (16k)
        and 32768 (32k).  Valid sizes for version 2 logs also
        include 65536 (64k), 131072 (128k) and 262144 (256k). The
        logbsize must be an integer multiple of the log
        stripe unit configured at mkfs time.

-Eric

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Dave Chinner]

On Fri, Jun 05, 2015 at 06:03:53PM -0500, Eric Sandeen wrote:
> On 6/5/15 5:22 PM, Dave Chinner wrote:
> 
> > On Fri, Jun 05, 2015 at 03:28:45PM +0200, Ales Novak wrote:
> >> However, this rule is only mentioned in the documentation, while it
> >> could be checked during the mount.
> > 
> > Where in the documentation is that mentioned?
> 
> Documentation/filesystems/xfs.txt:
> 
>   logbsize=value
>         Set the size of each in-memory log buffer.  The size may be
>         specified in bytes, or in kilobytes with a "k" suffix.
>         Valid sizes for version 1 and version 2 logs are 16384 (16k)
>         and 32768 (32k).  Valid sizes for version 2 logs also
>         include 65536 (64k), 131072 (128k) and 262144 (256k). The
>         logbsize must be an integer multiple of the log
>         stripe unit configured at mkfs time.

Ah, ok. I'll need to look at the history of that, because I think I
can see what it is intended to mean, but the "power-of-two" sizes
that are enforced will also enforce the "integer multiple" part,
too. I think it was more intended for people using wierd stripe
units (e.g. 96k) to say the equivalent on 2x96k is a valid log
buffer size. I suspect we need to revisit both the code and the
documentation here....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Ales Novak]

On 2015-6-6 00:22, Dave Chinner wrote:

> Can you give more information about the crashes? From this
> description, I do not know whether this is a work around or a fix
> because I don't know  what code is actually causing the problem or
> the circumstances in which the crash occurs. Hence I cannot
> determine if your change is the right change to make or whether the
> documetnation is simply wrong and we have a real bug we shoul dbe
> fixing.

The crashes occurred with logsunit=192k and logbsize=256k. xlog_sync() is 
calculating in logsunit units:

  count = XLOG_LSUNITTOB(log, XLOG_BTOLSUNIT(log, count_init));

With count_init=256k, the calculated roundoff will try to cover that in 
two 192k units. This roundoff is passed to the xlog_pack_data() which 
walks the buffer and (hopefully) fails when it hits its 256k border.

-- 
Ales Novak

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Mark Tinguely]

On 06/07/15 04:16, Ales Novak wrote:
>
> On 2015-6-6 00:22, Dave Chinner wrote:
>
>> Can you give more information about the crashes? From this
>> description, I do not know whether this is a work around or a fix
>> because I don't know what code is actually causing the problem or
>> the circumstances in which the crash occurs. Hence I cannot
>> determine if your change is the right change to make or whether the
>> documetnation is simply wrong and we have a real bug we shoul dbe
>> fixing.
>
> The crashes occurred with logsunit=192k and logbsize=256k. xlog_sync()
> is calculating in logsunit units:
>
> count = XLOG_LSUNITTOB(log, XLOG_BTOLSUNIT(log, count_init));
>
> With count_init=256k, the calculated roundoff will try to cover that in
> two 192k units. This roundoff is passed to the xlog_pack_data() which
> walks the buffer and (hopefully) fails when it hits its 256k border.
>

Talked about this once before:

  http://oss.sgi.com/archives/xfs/2013-03/msg00039.html

Mark.

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Ales Novak]

On 2015-6-8 15:04, Mark Tinguely wrote:

> On 06/07/15 04:16, Ales Novak wrote:
>> 
>> On 2015-6-6 00:22, Dave Chinner wrote:
>> 
>>> Can you give more information about the crashes? From this
>>> description, I do not know whether this is a work around or a fix
>>> because I don't know what code is actually causing the problem or
>>> the circumstances in which the crash occurs. Hence I cannot
>>> determine if your change is the right change to make or whether the
>>> documetnation is simply wrong and we have a real bug we shoul dbe
>>> fixing.
>> 
>> The crashes occurred with logsunit=192k and logbsize=256k. xlog_sync()
>> is calculating in logsunit units:
>> 
>> count = XLOG_LSUNITTOB(log, XLOG_BTOLSUNIT(log, count_init));
>> 
>> With count_init=256k, the calculated roundoff will try to cover that in
>> two 192k units. This roundoff is passed to the xlog_pack_data() which
>> walks the buffer and (hopefully) fails when it hits its 256k border.
>> 
>
> Talked about this once before:
>
> http://oss.sgi.com/archives/xfs/2013-03/msg00039.html
>
> Mark.
>

Thank you! I missed that.

Still I'd advocate for at least simple solution. Sure it's a root's fault 
if he tries to set logbsize not being integer multiple of logsunit, but 
crashing a kernel, that's bad...

-- 
Ales Novak

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

Re: [PATCH] xfs: fix in the setting of logbsize

[Ales Novak]

On 2015-6-7 00:32, Dave Chinner wrote:

> On Fri, Jun 05, 2015 at 06:03:53PM -0500, Eric Sandeen wrote:
>> On 6/5/15 5:22 PM, Dave Chinner wrote:
>>
>>> On Fri, Jun 05, 2015 at 03:28:45PM +0200, Ales Novak wrote:
>>>> However, this rule is only mentioned in the documentation, while it
>>>> could be checked during the mount.
>>>
>>> Where in the documentation is that mentioned?
>>
>> Documentation/filesystems/xfs.txt:
>>
>>   logbsize=value
>>         Set the size of each in-memory log buffer.  The size may be
>>         specified in bytes, or in kilobytes with a "k" suffix.
>>         Valid sizes for version 1 and version 2 logs are 16384 (16k)
>>         and 32768 (32k).  Valid sizes for version 2 logs also
>>         include 65536 (64k), 131072 (128k) and 262144 (256k). The
>>         logbsize must be an integer multiple of the log
>>         stripe unit configured at mkfs time.
>
> Ah, ok. I'll need to look at the history of that, because I think I
> can see what it is intended to mean, but the "power-of-two" sizes
> that are enforced will also enforce the "integer multiple" part,
> too. I think it was more intended for people using wierd stripe
> units (e.g. 96k) to say the equivalent on 2x96k is a valid log
> buffer size. I suspect we need to revisit both the code and the
> documentation here....

So, how will we proceed here? I understand that my patch only somehow 
blindly implements the restriction mentioned in the documentation and that 
proper solution would mean completely reworking these restrictions; 
however I don't like the state where setting seemingly harmless 
option leads to kernel crash (and possibly fs corruption).

While I understand that only playful root would do that.

-- 
Ales Novak

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