[Drbd-dev] [PATCH] drbd: Fix exception handling in nla_put_drbd_cfg_context()

[Drbd-dev] [PATCH] drbd: Fix exception handling in nla_put_drbd_cfg_context()

[Markus Elfring]

Date: Fri, 17 Mar 2023 18:32:05 +0100

The label “nla_put_failure” was used to jump to another pointer check
despite of the detail in the implementation of the function
“nla_put_drbd_cfg_context” that it was determined already that
the corresponding variable contained a null pointer.

* Thus return directly after a call of the function
  “nla_nest_start_noflag” failed.

* Delete an extra pointer check which became unnecessary
  with this refactoring.


This issue was detected by using the Coccinelle software.

Fixes: 543cc10b4cc5c60aa9fcc62705ccfb9998bf4697 ("drbd: drbd_adm_get_status needs to show some more detail")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/drbd/drbd_nl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index f49f2a5282e1..9cb947127472 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3187,7 +3187,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
     struct nlattr *nla;
     nla = nla_nest_start_noflag(skb, DRBD_NLA_CFG_CONTEXT);
     if (!nla)
-        goto nla_put_failure;
+        return -EMSGSIZE;
     if (device &&
         nla_put_u32(skb, T_ctx_volume, device->vnr))
         goto nla_put_failure;
@@ -3205,8 +3205,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
     return 0;
 
 nla_put_failure:
-    if (nla)
-        nla_nest_cancel(skb, nla);
+    nla_nest_cancel(skb, nla);
     return -EMSGSIZE;
 }
 
--
2.40.0

[Drbd-dev] [PATCH resent] drbd: Fix exception handling in nla_put_drbd_cfg_context()

[Markus Elfring]

Date: Fri, 17 Mar 2023 18:32:05 +0100

The label “nla_put_failure” was used to jump to another pointer check
despite of the detail in the implementation of the function
“nla_put_drbd_cfg_context” that it was determined already that
the corresponding variable contained a null pointer.

* Thus return directly after a call of the function
  “nla_nest_start_noflag” failed.

* Delete an extra pointer check which became unnecessary
  with this refactoring.


This issue was detected by using the Coccinelle software.

Fixes: 543cc10b4cc5c60aa9fcc62705ccfb9998bf4697 ("drbd: drbd_adm_get_status needs to show some more detail")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/drbd/drbd_nl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index f49f2a5282e1..9cb947127472 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3187,7 +3187,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
 	struct nlattr *nla;
 	nla = nla_nest_start_noflag(skb, DRBD_NLA_CFG_CONTEXT);
 	if (!nla)
-		goto nla_put_failure;
+		return -EMSGSIZE;
 	if (device &&
 	    nla_put_u32(skb, T_ctx_volume, device->vnr))
 		goto nla_put_failure;
@@ -3205,8 +3205,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
 	return 0;

 nla_put_failure:
-	if (nla)
-		nla_nest_cancel(skb, nla);
+	nla_nest_cancel(skb, nla);
 	return -EMSGSIZE;
 }

--
2.40.0

Re: [Drbd-dev] [PATCH resent] drbd: Fix exception handling in nla_put_drbd_cfg_context()

[Christoph Böhmwalder]

Am 25.03.23 um 15:07 schrieb Markus Elfring:
> Date: Fri, 17 Mar 2023 18:32:05 +0100
> 
> The label “nla_put_failure” was used to jump to another pointer check
> despite of the detail in the implementation of the function
> “nla_put_drbd_cfg_context” that it was determined already that
> the corresponding variable contained a null pointer.
> 
> * Thus return directly after a call of the function
>   “nla_nest_start_noflag” failed.
> 
> * Delete an extra pointer check which became unnecessary
>   with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 543cc10b4cc5c60aa9fcc62705ccfb9998bf4697 ("drbd: drbd_adm_get_status needs to show some more detail")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/drbd/drbd_nl.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
> index f49f2a5282e1..9cb947127472 100644
> --- a/drivers/block/drbd/drbd_nl.c
> +++ b/drivers/block/drbd/drbd_nl.c
> @@ -3187,7 +3187,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
>  	struct nlattr *nla;
>  	nla = nla_nest_start_noflag(skb, DRBD_NLA_CFG_CONTEXT);
>  	if (!nla)
> -		goto nla_put_failure;
> +		return -EMSGSIZE;
>  	if (device &&
>  	    nla_put_u32(skb, T_ctx_volume, device->vnr))
>  		goto nla_put_failure;
> @@ -3205,8 +3205,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
>  	return 0;
> 
>  nla_put_failure:
> -	if (nla)
> -		nla_nest_cancel(skb, nla);
> +	nla_nest_cancel(skb, nla);
>  	return -EMSGSIZE;
>  }
> 
> --
> 2.40.0
> 

Sorry, I fail to see how this is an improvement over the status quo,
much less a "fix".

Can you identify the issue with the current code and can you explain how
your patch makes it better?

-- 
Christoph Böhmwalder
LINBIT | Keeping the Digital World Running
DRBD HA —  Disaster Recovery — Software defined Storage

Re: [Drbd-dev] drbd: Fix exception handling in nla_put_drbd_cfg_context()

[Markus Elfring]

> Can you identify the issue with the current code and can you explain how
> your patch makes it better?

I propose to avoid a duplicate pointer check also in this function implementation.

Regards,
Markus