[PATCH 1/1] block: add default clause for unsupported T10_PI types

[PATCH 1/1] block: add default clause for unsupported T10_PI types

[Max Gurtovoy]

kbuild robot reported the following warning:

block/t10-pi.c: In function 't10_pi_verify':
block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION'
                     not handled in switch [-Wswitch]
      switch (type) {
      ^~~~~~

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
 block/t10-pi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/t10-pi.c b/block/t10-pi.c
index 0c0120a..57f304a 100644
--- a/block/t10-pi.c
+++ b/block/t10-pi.c
@@ -79,6 +79,9 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter,
 			    pi->ref_tag == T10_PI_REF_ESCAPE)
 				goto next;
 			break;
+		default:
+			/* Currently we support only TYPE1/2/3 */
+			BUG();
 		}
 
 		csum = fn(iter->data_buf, iter->interval);
-- 
1.8.3.1

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Jens Axboe]

On 9/21/19 4:00 PM, Max Gurtovoy wrote:
> kbuild robot reported the following warning:
> 
> block/t10-pi.c: In function 't10_pi_verify':
> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION'
>                       not handled in switch [-Wswitch]
>        switch (type) {
>        ^~~~~~

This commit message is woefully lacking. It doesn't explain anything...?
Why aren't we just flagging this as an error? Seems a lot saner than adding
a BUG().


diff --git a/block/t10-pi.c b/block/t10-pi.c
index 0c0120a672f9..6a1d4128a9d4 100644
--- a/block/t10-pi.c
+++ b/block/t10-pi.c
@@ -79,6 +79,10 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter,
 			    pi->ref_tag == T10_PI_REF_ESCAPE)
 				goto next;
 			break;
+		default:
+			pr_err("%s: unsupported protection type: %d\n",
+						iter->disk_name, type);
+			return BLK_STS_PROTECTION;
 		}
 
 		csum = fn(iter->data_buf, iter->interval);

-- 
Jens Axboe

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Martin K. Petersen]

Jens,

>> block/t10-pi.c: In function 't10_pi_verify':
>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION'
>>                       not handled in switch [-Wswitch]
>>        switch (type) {
>>        ^~~~~~
>
> This commit message is woefully lacking. It doesn't explain
> anything...?  Why aren't we just flagging this as an error? Seems a
> lot saner than adding a BUG().

The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached
protection information". So it's a block layer bug if we ever end up in
this function and the protection type is 0.

My main beef with all this is that I don't particularly like introducing
a nonsensical switch case to quiesce a compiler warning. We never call
t10_pi_verify() with a type of 0 and there are lots of safeguards
further up the stack preventing that from ever happening. Adding a Type
0 here gives the reader the false impression that it's valid input to
the function. Which it really isn't.

Arnd: Any ideas how to handle this?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Jens Axboe]

On 9/21/19 4:54 PM, Martin K. Petersen wrote:
> 
> Jens,
> 
>>> block/t10-pi.c: In function 't10_pi_verify':
>>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION'
>>>                        not handled in switch [-Wswitch]
>>>         switch (type) {
>>>         ^~~~~~
>>
>> This commit message is woefully lacking. It doesn't explain
>> anything...?  Why aren't we just flagging this as an error? Seems a
>> lot saner than adding a BUG().
> 
> The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached
> protection information". So it's a block layer bug if we ever end up in
> this function and the protection type is 0.
> 
> My main beef with all this is that I don't particularly like introducing
> a nonsensical switch case to quiesce a compiler warning. We never call
> t10_pi_verify() with a type of 0 and there are lots of safeguards
> further up the stack preventing that from ever happening. Adding a Type
> 0 here gives the reader the false impression that it's valid input to
> the function. Which it really isn't.
> 
> Arnd: Any ideas how to handle this?

Why not just add the default catch, that logs, and returns the error?
Would seem like the cleanest way to handle it to me. Since the
compiler knows the type, it'll complain if we have missing cases.

-- 
Jens Axboe

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Max Gurtovoy]

On 9/22/2019 2:29 AM, Jens Axboe wrote:
> On 9/21/19 4:54 PM, Martin K. Petersen wrote:
>> Jens,
>>
>>>> block/t10-pi.c: In function 't10_pi_verify':
>>>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION'
>>>>                         not handled in switch [-Wswitch]
>>>>          switch (type) {
>>>>          ^~~~~~
>>> This commit message is woefully lacking. It doesn't explain
>>> anything...?  Why aren't we just flagging this as an error? Seems a
>>> lot saner than adding a BUG().
>> The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached
>> protection information". So it's a block layer bug if we ever end up in
>> this function and the protection type is 0.
>>
>> My main beef with all this is that I don't particularly like introducing
>> a nonsensical switch case to quiesce a compiler warning. We never call
>> t10_pi_verify() with a type of 0 and there are lots of safeguards
>> further up the stack preventing that from ever happening. Adding a Type
>> 0 here gives the reader the false impression that it's valid input to
>> the function. Which it really isn't.
>>
>> Arnd: Any ideas how to handle this?
> Why not just add the default catch, that logs, and returns the error?
> Would seem like the cleanest way to handle it to me. Since the
> compiler knows the type, it'll complain if we have missing cases.

what about removing the switch/case and do the following change:

diff --git a/block/t10-pi.c b/block/t10-pi.c
index 0c0120a..9803c7e 100644
--- a/block/t10-pi.c
+++ b/block/t10-pi.c
@@ -55,13 +55,14 @@ static blk_status_t t10_pi_verify(struct 
blk_integrity_iter *iter,
  {
         unsigned int i;

+       BUG_ON(type == T10_PI_TYPE0_PROTECTION);
+
         for (i = 0 ; i < iter->data_size ; i += iter->interval) {
                 struct t10_pi_tuple *pi = iter->prot_buf;
                 __be16 csum;

-               switch (type) {
-               case T10_PI_TYPE1_PROTECTION:
-               case T10_PI_TYPE2_PROTECTION:
+               if (type == T10_PI_TYPE1_PROTECTION ||
+                   type == T10_PI_TYPE2_PROTECTION) {
                         if (pi->app_tag == T10_PI_APP_ESCAPE)
                                 goto next;

@@ -73,12 +74,10 @@ static blk_status_t t10_pi_verify(struct 
blk_integrity_iter *iter,
                                        iter->seed, 
be32_to_cpu(pi->ref_tag));
                                 return BLK_STS_PROTECTION;
                         }
-                       break;
-               case T10_PI_TYPE3_PROTECTION:
+               } else if (type == T10_PI_TYPE3_PROTECTION) {
                         if (pi->app_tag == T10_PI_APP_ESCAPE &&
                             pi->ref_tag == T10_PI_REF_ESCAPE)
                                 goto next;
-                       break;
                 }

                 csum = fn(iter->data_buf, iter->interval);

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Jens Axboe]

On 9/22/19 3:38 AM, Max Gurtovoy wrote:
> 
> On 9/22/2019 2:29 AM, Jens Axboe wrote:
>> On 9/21/19 4:54 PM, Martin K. Petersen wrote:
>>> Jens,
>>>
>>>>> block/t10-pi.c: In function 't10_pi_verify':
>>>>> block/t10-pi.c:62:3: warning: enumeration value 'T10_PI_TYPE0_PROTECTION'
>>>>>                          not handled in switch [-Wswitch]
>>>>>           switch (type) {
>>>>>           ^~~~~~
>>>> This commit message is woefully lacking. It doesn't explain
>>>> anything...?  Why aren't we just flagging this as an error? Seems a
>>>> lot saner than adding a BUG().
>>> The fundamental issue is that T10_PI_TYPE0_PROTECTION means "no attached
>>> protection information". So it's a block layer bug if we ever end up in
>>> this function and the protection type is 0.
>>>
>>> My main beef with all this is that I don't particularly like introducing
>>> a nonsensical switch case to quiesce a compiler warning. We never call
>>> t10_pi_verify() with a type of 0 and there are lots of safeguards
>>> further up the stack preventing that from ever happening. Adding a Type
>>> 0 here gives the reader the false impression that it's valid input to
>>> the function. Which it really isn't.
>>>
>>> Arnd: Any ideas how to handle this?
>> Why not just add the default catch, that logs, and returns the error?
>> Would seem like the cleanest way to handle it to me. Since the
>> compiler knows the type, it'll complain if we have missing cases.
> 
> what about removing the switch/case and do the following change:

It's effectively the same thing, I really don't think we need (or should
have) a BUG/BUG_ON for this condition. Just return an error?

Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log
and return an error. Add a comment on how it's impossible, if need be.
I don't think it has to be more complicated than that.

-- 
Jens Axboe

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Martin K. Petersen]

Jens,

> It's effectively the same thing, I really don't think we need (or should
> have) a BUG/BUG_ON for this condition. Just return an error?

> Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log
> and return an error. Add a comment on how it's impossible, if need be.
> I don't think it has to be more complicated than that.

The additional case statement is inside an iterator loop which would
bomb for Type 0 since there is no protection buffer to iterate
over. We'd presumably never reach that default: case before
dereferencing something bad.

t10_pi_verify() is a static function exclusively called by helpers that
pass in either 1 or 3 as argument. It seems kind of silly that we have
to jump through hoops to silence a compiler warning for this. I would
prefer a BUILD_BUG_ON(type == T10_PI_TYPE0_PROTECTION) at the top of the
function but that does not satisfy the -Wswitch logic either.

Anyway. Enough energy wasted on this. I'm OK with either the default:
case or Max' if statement approach. My objection is purely
wrt. introducing semantically incorrect and/or unreachable code to
silence compiler warnings. Seems backwards.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Max Gurtovoy]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]


On 9/22/2019 8:31 PM, Martin K. Petersen wrote:
> Jens,
>
>> It's effectively the same thing, I really don't think we need (or should
>> have) a BUG/BUG_ON for this condition. Just return an error?
>> Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log
>> and return an error. Add a comment on how it's impossible, if need be.
>> I don't think it has to be more complicated than that.
> The additional case statement is inside an iterator loop which would
> bomb for Type 0 since there is no protection buffer to iterate
> over. We'd presumably never reach that default: case before
> dereferencing something bad.
>
> t10_pi_verify() is a static function exclusively called by helpers that
> pass in either 1 or 3 as argument. It seems kind of silly that we have
> to jump through hoops to silence a compiler warning for this. I would
> prefer a BUILD_BUG_ON(type == T10_PI_TYPE0_PROTECTION) at the top of the
> function but that does not satisfy the -Wswitch logic either.
>
> Anyway. Enough energy wasted on this. I'm OK with either the default:
> case or Max' if statement approach. My objection is purely
> wrt. introducing semantically incorrect and/or unreachable code to
> silence compiler warnings. Seems backwards.

I agree that enough energy wasted here :)

Attached some proposal to fix this warning.

Let me know if you want me to send it to the mailing list


[-- Attachment #2: 0001-block-t10-pi-fix-Wswitch-warning.patch --]
[-- Type: text/plain, Size: 1957 bytes --]

From 058b2e2da4ada6d27287533a7228abd80de17248 Mon Sep 17 00:00:00 2001
From: Max Gurtovoy <maxg@mellanox.com>
Date: Sun, 22 Sep 2019 12:46:55 +0300
Subject: [PATCH 1/1] block: t10-pi: fix -Wswitch warning

Changing the switch() statement to symbolic constants made the compiler
(at least clang-9, did not check gcc) notice that there is one enum value
that is not handled here:

block/t10-pi.c:62:11: error: enumeration value 'T10_PI_TYPE0_PROTECTION'
not handled in switch [-Werror,-Wswitch]

Add a BUG_ON statement if we ever get to t10_pi_verify function with
TYPE0 and replace the switch() statement with if/else clause for the
valid types.

Fixes: 9b2061b1a262 ("block: use symbolic constants for t10_pi type")
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
 block/t10-pi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/t10-pi.c b/block/t10-pi.c
index 0c0120a..9803c7e 100644
--- a/block/t10-pi.c
+++ b/block/t10-pi.c
@@ -55,13 +55,14 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter,
 {
 	unsigned int i;
 
+	BUG_ON(type == T10_PI_TYPE0_PROTECTION);
+
 	for (i = 0 ; i < iter->data_size ; i += iter->interval) {
 		struct t10_pi_tuple *pi = iter->prot_buf;
 		__be16 csum;
 
-		switch (type) {
-		case T10_PI_TYPE1_PROTECTION:
-		case T10_PI_TYPE2_PROTECTION:
+		if (type == T10_PI_TYPE1_PROTECTION ||
+		    type == T10_PI_TYPE2_PROTECTION) {
 			if (pi->app_tag == T10_PI_APP_ESCAPE)
 				goto next;
 
@@ -73,12 +74,10 @@ static blk_status_t t10_pi_verify(struct blk_integrity_iter *iter,
 				       iter->seed, be32_to_cpu(pi->ref_tag));
 				return BLK_STS_PROTECTION;
 			}
-			break;
-		case T10_PI_TYPE3_PROTECTION:
+		} else if (type == T10_PI_TYPE3_PROTECTION) {
 			if (pi->app_tag == T10_PI_APP_ESCAPE &&
 			    pi->ref_tag == T10_PI_REF_ESCAPE)
 				goto next;
-			break;
 		}
 
 		csum = fn(iter->data_buf, iter->interval);
-- 
1.8.3.1

Re: [PATCH 1/1] block: add default clause for unsupported T10_PI types

[Jens Axboe]

On 9/22/19 3:21 PM, Max Gurtovoy wrote:
> 
> On 9/22/2019 8:31 PM, Martin K. Petersen wrote:
>> Jens,
>>
>>> It's effectively the same thing, I really don't think we need (or should
>>> have) a BUG/BUG_ON for this condition. Just return an error?
>>> Just include a T10_PI_TYPE0_PROTECTION case in the switch, have it log
>>> and return an error. Add a comment on how it's impossible, if need be.
>>> I don't think it has to be more complicated than that.
>> The additional case statement is inside an iterator loop which would
>> bomb for Type 0 since there is no protection buffer to iterate
>> over. We'd presumably never reach that default: case before
>> dereferencing something bad.
>>
>> t10_pi_verify() is a static function exclusively called by helpers that
>> pass in either 1 or 3 as argument. It seems kind of silly that we have
>> to jump through hoops to silence a compiler warning for this. I would
>> prefer a BUILD_BUG_ON(type == T10_PI_TYPE0_PROTECTION) at the top of the
>> function but that does not satisfy the -Wswitch logic either.
>>
>> Anyway. Enough energy wasted on this. I'm OK with either the default:
>> case or Max' if statement approach. My objection is purely
>> wrt. introducing semantically incorrect and/or unreachable code to
>> silence compiler warnings. Seems backwards.
> 
> I agree that enough energy wasted here :)

Agree ;-)

> Attached some proposal to fix this warning.
> 
> Let me know if you want me to send it to the mailing list

OK, fine with me, I've queued it up.

-- 
Jens Axboe