Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel

[Paul Moore <paul.moore@hp.com>]

On Fri, 2011-01-28 at 15:17 +0100, Julia Lawall wrote:
> genlmsg_cancel subtracts some constants from its second argument before
> calling nlmsg_cancel.  nlmsg_cancel then calls nlmsg_trim on the same
> arguments.  nlmsg_trim tests for NULL before doing any computation, but a
> NULL second argument to genlmsg_cancel is no longer NULL due to the initial
> subtraction.  Nothing else happens in this execution, so the call to
> genlmsg_cancel is simply unnecessary in this case.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression data;
> @@
> 
> if (data = NULL) { ...
> * genlmsg_cancel(..., data);
>   ...
>   return ...;
> }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

In all of the cases below, these functions are called multiple times to
generate data chunks (additional netlink attributes) which are appended
to an existing skbuff.  I believe that the calls to genlmsg_cancel() are
still needed to help cleanup in the case where the functions fail on the
Nth call.

If I'm wrong, feel free to enlighten me.

> ---
>  net/netlabel/netlabel_cipso_v4.c  |    2 +-
>  net/netlabel/netlabel_mgmt.c      |    4 ++--
>  net/netlabel/netlabel_unlabeled.c |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
> index 5f14c84..0a1f77b 100644
> --- a/net/netlabel/netlabel_cipso_v4.c
> +++ b/net/netlabel/netlabel_cipso_v4.c
> @@ -635,7 +635,7 @@ static int netlbl_cipsov4_listall_cb(struct cipso_v4_doi *doi_def, void *arg)
>  			   cb_arg->seq, &netlbl_cipsov4_gnl_family,
>  			   NLM_F_MULTI, NLBL_CIPSOV4_C_LISTALL);
>  	if (data = NULL)
> -		goto listall_cb_failure;
> +		return ret_val;
>  
>  	ret_val = nla_put_u32(cb_arg->skb, NLBL_CIPSOV4_A_DOI, doi_def->doi);
>  	if (ret_val != 0)
> diff --git a/net/netlabel/netlabel_mgmt.c b/net/netlabel/netlabel_mgmt.c
> index 998e85e..daaa01d 100644
> --- a/net/netlabel/netlabel_mgmt.c
> +++ b/net/netlabel/netlabel_mgmt.c
> @@ -452,7 +452,7 @@ static int netlbl_mgmt_listall_cb(struct netlbl_dom_map *entry, void *arg)
>  			   cb_arg->seq, &netlbl_mgmt_gnl_family,
>  			   NLM_F_MULTI, NLBL_MGMT_C_LISTALL);
>  	if (data = NULL)
> -		goto listall_cb_failure;
> +		return ret_val;
>  
>  	ret_val = netlbl_mgmt_listentry(cb_arg->skb, entry);
>  	if (ret_val != 0)
> @@ -617,7 +617,7 @@ static int netlbl_mgmt_protocols_cb(struct sk_buff *skb,
>  			   &netlbl_mgmt_gnl_family, NLM_F_MULTI,
>  			   NLBL_MGMT_C_PROTOCOLS);
>  	if (data = NULL)
> -		goto protocols_cb_failure;
> +		return ret_val;
>  
>  	ret_val = nla_put_u32(skb, NLBL_MGMT_A_PROTOCOL, protocol);
>  	if (ret_val != 0)
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index e2b0a68..b5d3945 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -1141,7 +1141,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
>  			   cb_arg->seq, &netlbl_unlabel_gnl_family,
>  			   NLM_F_MULTI, cmd);
>  	if (data = NULL)
> -		goto list_cb_failure;
> +		return ret_val;
>  
>  	if (iface->ifindex > 0) {
>  		dev = dev_get_by_index(&init_net, iface->ifindex);
> 

-- 
paul moore
linux @ hp