[PATCH 1/1] dm raid: fix compat_features validation

[PATCH 1/1] dm raid: fix compat_features validation

[Andy Whitcroft]

In commit ecbfb9f118bce4 ("dm raid: add raid level takeover support") a new
compatible feature flag was added.  Validation for these compat_features
was added but this only passes for new raid mappings with this feature
flag.  This causes previously created raid mappings to be failed at import.

Check compat_features for any valid combinations.

Fixes: ecbfb9f118bce4 ("dm raid: add raid level takeover support")
BugLink: http://bugs.launchpad.net/bugs/1631298
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/md/dm-raid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

It very much looks like these are intended to be optional extended feature
flags.  That we should be accepting any valid flag and rejecting any bit
not in that set.  We should however not be ensuring that specific bits
are actually set.  Certainly as things stand raid sets built on previous
kernel versions cannot be assembled.

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 8abde6b..6ddea60 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -2258,7 +2258,7 @@ static int super_validate(struct raid_set *rs, struct md_rdev *rdev)
 	if (!mddev->events && super_init_validation(rs, rdev))
 		return -EINVAL;
 
-	if (le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
+	if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {
 		rs->ti->error = "Unable to assemble array: Unknown flag(s) in compatible feature flags";
 		return -EINVAL;
 	}
-- 
2.9.3

Re: [PATCH 1/1] dm raid: fix compat_features validation

[Mike Snitzer]

On Tue, Oct 11 2016 at 10:28am -0400,
Andy Whitcroft <apw@canonical.com> wrote:

> In commit ecbfb9f118bce4 ("dm raid: add raid level takeover support") a new
> compatible feature flag was added.  Validation for these compat_features
> was added but this only passes for new raid mappings with this feature
> flag.  This causes previously created raid mappings to be failed at import.
> 
> Check compat_features for any valid combinations.
> 
> Fixes: ecbfb9f118bce4 ("dm raid: add raid level takeover support")
> BugLink: http://bugs.launchpad.net/bugs/1631298
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/md/dm-raid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> It very much looks like these are intended to be optional extended feature
> flags.  That we should be accepting any valid flag and rejecting any bit
> not in that set.  We should however not be ensuring that specific bits
> are actually set.  Certainly as things stand raid sets built on previous
> kernel versions cannot be assembled.

Right, your patch looks good to me.  But I'll wait for confirmation from
Heinz before I stage your fix.

Thanks,
Mike

Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation

[Heinz Mauelshagen]

Andy,

good catch.

We should rather check for  V190 support only in case any
compat feature flags are actually set.

I.e.:

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 8abde6b..2a39700 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -2258,7 +2258,8 @@ static int super_validate(struct raid_set *rs, 
struct md_rdev *rdev)
         if (!mddev->events && super_init_validation(rs, rdev))
                 return -EINVAL;

-       if (le32_to_cpu(sb->compat_features) != 
FEATURE_FLAG_SUPPORTS_V190) {
+       if (le32_to_cpu(sb->compat_features) &&
+           le32_to_cpu(sb->compat_features) != 
FEATURE_FLAG_SUPPORTS_V190) {
                 rs->ti->error = "Unable to assemble array: Unknown 
flag(s) in compatible feature flags";
                 return -EINVAL;
         }

On 10/11/2016 04:28 PM, Andy Whitcroft wrote:
> In commit ecbfb9f118bce4 ("dm raid: add raid level takeover support") a new
> compatible feature flag was added.  Validation for these compat_features
> was added but this only passes for new raid mappings with this feature
> flag.  This causes previously created raid mappings to be failed at import.
>
> Check compat_features for any valid combinations.
>
> Fixes: ecbfb9f118bce4 ("dm raid: add raid level takeover support")
> BugLink: http://bugs.launchpad.net/bugs/1631298
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>   drivers/md/dm-raid.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> It very much looks like these are intended to be optional extended feature
> flags.  That we should be accepting any valid flag and rejecting any bit
> not in that set.  We should however not be ensuring that specific bits
> are actually set.  Certainly as things stand raid sets built on previous
> kernel versions cannot be assembled.
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 8abde6b..6ddea60 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -2258,7 +2258,7 @@ static int super_validate(struct raid_set *rs, struct md_rdev *rdev)
>   	if (!mddev->events && super_init_validation(rs, rdev))
>   		return -EINVAL;
>   
> -	if (le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
> +	if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {
>   		rs->ti->error = "Unable to assemble array: Unknown flag(s) in compatible feature flags";
>   		return -EINVAL;
>   	}

Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation

[Andy Whitcroft]

On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote:
> 
> Andy,
> 
> good catch.
> 
> We should rather check for  V190 support only in case any
> compat feature flags are actually set.
> 
> {
> +       if (le32_to_cpu(sb->compat_features) &&
> +           le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190)
> {
>                 rs->ti->error = "Unable to assemble array: Unknown flag(s)
> in compatible feature flags";
>                 return -EINVAL;
>         }

If the feature flags are single bit combinations then I believe the
below does check exactly that.  Checking for no 1s outside of the
expected features, caring not for the value of the valid bits:

+     if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {

with the possibilty to or in additional feature bits as they are added.

-apw

Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation

[Heinz Mauelshagen]

On 10/11/2016 05:38 PM, Andy Whitcroft wrote:
> On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote:
>> Andy,
>>
>> good catch.
>>
>> We should rather check for  V190 support only in case any
>> compat feature flags are actually set.
>>
>> {
>> +       if (le32_to_cpu(sb->compat_features) &&
>> +           le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190)
>> {
>>                  rs->ti->error = "Unable to assemble array: Unknown flag(s)
>> in compatible feature flags";
>>                  return -EINVAL;
>>          }
> If the feature flags are single bit combinations then I believe the
> below does check exactly that.  Checking for no 1s outside of the
> expected features, caring not for the value of the valid bits:
>
> +     if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {
>
> with the possibilty to or in additional feature bits as they are added.

Thanks,
I prefer this to be easier readable.

>
> -apw
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/1 V2] dm raid: fix compat_features validation

[Andy Whitcroft]

>From a30fba068e41214cb0ffcb14e68722482765e0c9 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <apw@canonical.com>
Date: Tue, 11 Oct 2016 15:16:57 +0100

In ecbfb9f118bce4 ("dm raid: add raid level takeover support") a new
compatible feature flag was added.  Validation for these compat_features
was added but this only passes for new raid mappings with this feature
flag.  This causes previously created raid mappings to be failed at
import.

Check compat_features for the only valid combination.

Fixes: ecbfb9f118bce4 ("dm raid: add raid level takeover support")
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/md/dm-raid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

V2: simplify checks as per maintainer.

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 8abde6b..2a39700 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -2258,7 +2258,8 @@ static int super_validate(struct raid_set *rs, struct md_rdev *rdev)
 	if (!mddev->events && super_init_validation(rs, rdev))
 		return -EINVAL;
 
-	if (le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
+	if (le32_to_cpu(sb->compat_features) &&
+	    le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
 		rs->ti->error = "Unable to assemble array: Unknown flag(s) in compatible feature flags";
 		return -EINVAL;
 	}
-- 
2.9.3

Re: [PATCH 1/1 V2] dm raid: fix compat_features validation

[Heinz Mauelshagen]

Acked-by: Heinz Mauelshagen <heinzm@redhat.com>

On 10/11/2016 06:21 PM, Andy Whitcroft wrote:
>  From a30fba068e41214cb0ffcb14e68722482765e0c9 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <apw@canonical.com>
> Date: Tue, 11 Oct 2016 15:16:57 +0100
>
> In ecbfb9f118bce4 ("dm raid: add raid level takeover support") a new
> compatible feature flag was added.  Validation for these compat_features
> was added but this only passes for new raid mappings with this feature
> flag.  This causes previously created raid mappings to be failed at
> import.

Clarification:
to allow for feature checks, the compat_features member was
in the dm-raid superblock from the beginning (so before ecbfb9f118bce4).
It got renamed from features to compat_features because incompat_features
got introduced with that commit together with the problematic check
of compat_features you thankfully found.


>
> Check compat_features for the only valid combination.
>
> Fixes: ecbfb9f118bce4 ("dm raid: add raid level takeover support")
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>   drivers/md/dm-raid.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> V2: simplify checks as per maintainer.
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 8abde6b..2a39700 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -2258,7 +2258,8 @@ static int super_validate(struct raid_set *rs, struct md_rdev *rdev)
>   	if (!mddev->events && super_init_validation(rs, rdev))
>   		return -EINVAL;
>   
> -	if (le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
> +	if (le32_to_cpu(sb->compat_features) &&
> +	    le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
>   		rs->ti->error = "Unable to assemble array: Unknown flag(s) in compatible feature flags";
>   		return -EINVAL;
>   	}

Re: [PATCH 1/1] dm raid: fix compat_features validation

[Mike Snitzer]

On Tue, Oct 11 2016 at 11:44am -0400,
Heinz Mauelshagen <heinzm@redhat.com> wrote:

> 
> 
> On 10/11/2016 05:38 PM, Andy Whitcroft wrote:
> >On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote:
> >>Andy,
> >>
> >>good catch.
> >>
> >>We should rather check for  V190 support only in case any
> >>compat feature flags are actually set.
> >>
> >>{
> >>+       if (le32_to_cpu(sb->compat_features) &&
> >>+           le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190)
> >>{
> >>                 rs->ti->error = "Unable to assemble array: Unknown flag(s)
> >>in compatible feature flags";
> >>                 return -EINVAL;
> >>         }
> >If the feature flags are single bit combinations then I believe the
> >below does check exactly that.  Checking for no 1s outside of the
> >expected features, caring not for the value of the valid bits:
> >
> >+     if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {
> >
> >with the possibilty to or in additional feature bits as they are added.
> 
> Thanks,
> I prefer this to be easier readable.

Readable or not, the code with the != is _not_ future-proof.  Whereas
Andy's solution is.  If/when a new compat feature comes along then
FEATURE_FLAG_SUPPORTS_V190 would be replaced to be a macro that ORs all
the new compat features together (e.g. FEATURE_FLAG_COMPAT).  E.g. how
dm-thin-metadata.c:__check_incompat_features() does.

We can go with the != code for now, since any future changes would
likely cause this test to be changed.  Or we could fix it now _for
real_.

Mike

Re: [PATCH 1/1] dm raid: fix compat_features validation

[Heinz Mauelshagen]

On 10/11/2016 07:44 PM, Mike Snitzer wrote:
> On Tue, Oct 11 2016 at 11:44am -0400,
> Heinz Mauelshagen <heinzm@redhat.com> wrote:
>
>>
>> On 10/11/2016 05:38 PM, Andy Whitcroft wrote:
>>> On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote:
>>>> Andy,
>>>>
>>>> good catch.
>>>>
>>>> We should rather check for  V190 support only in case any
>>>> compat feature flags are actually set.
>>>>
>>>> {
>>>> +       if (le32_to_cpu(sb->compat_features) &&
>>>> +           le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190)
>>>> {
>>>>                  rs->ti->error = "Unable to assemble array: Unknown flag(s)
>>>> in compatible feature flags";
>>>>                  return -EINVAL;
>>>>          }
>>> If the feature flags are single bit combinations then I believe the
>>> below does check exactly that.  Checking for no 1s outside of the
>>> expected features, caring not for the value of the valid bits:
>>>
>>> +     if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) {
>>>
>>> with the possibilty to or in additional feature bits as they are added.
>> Thanks,
>> I prefer this to be easier readable.
> Readable or not, the code with the != is _not_ future-proof.  Whereas
> Andy's solution is.  If/when a new compat feature comes along then
> FEATURE_FLAG_SUPPORTS_V190 would be replaced to be a macro that ORs all
> the new compat features together (e.g. FEATURE_FLAG_COMPAT).  E.g. how
> dm-thin-metadata.c:__check_incompat_features() does.

If we'll have to introduce more feature flags in the future (e.g. for 
clustered raid1
support), this is going to be based on the test_bit() API for consistency
with any other flag processing we do in the target.

Heinz

> We can go with the != code for now, since any future changes would
> likely cause this test to be changed.  Or we could fix it now _for
> real_.
>
> Mike