[1/1] dm raid: fix compat_features validation
diff mbox

Message ID 1c517f14-1234-7844-fc6a-cd1b9698fb8b@redhat.com
State Superseded, archived
Delegated to: Mike Snitzer
Headers show

Commit Message

Heinz Mauelshagen Oct. 11, 2016, 3:04 p.m. UTC
Andy,

good catch.

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

I.e.:

                 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;
>   	}

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

Comments

Andy Whitcroft Oct. 11, 2016, 3:38 p.m. UTC | #1
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Heinz Mauelshagen Oct. 11, 2016, 3:44 p.m. UTC | #2
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer Oct. 11, 2016, 5:44 p.m. UTC | #3
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Heinz Mauelshagen Oct. 14, 2016, 5:14 p.m. UTC | #4
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

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

Patch
diff mbox

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) {