Re: [dm-devel] [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  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

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

[Mike Snitzer]

On Tue, Oct 11 2016 at 11:44am -0400,
Heinz Mauelshagen  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

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


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

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

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

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

[Mike Snitzer]

On Tue, Oct 11 2016 at 10:28am -0400,
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 
> ---
>  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

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

[dm-devel] [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 
---
 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

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