All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 14:17 ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-01-28 13:58 UTC (permalink / raw)
  To: Paul Moore; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

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>

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 14:17 ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-01-28 14:17 UTC (permalink / raw)
  To: Paul Moore; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

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>

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
  2011-01-28 14:17 ` Julia Lawall
@ 2011-01-28 14:36   ` Paul Moore
  -1 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2011-01-28 14:36 UTC (permalink / raw)
  To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 14:36   ` Paul Moore
  0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2011-01-28 14:36 UTC (permalink / raw)
  To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
  2011-01-28 14:36   ` Paul Moore
@ 2011-01-28 14:58     ` Julia Lawall
  -1 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-01-28 14:58 UTC (permalink / raw)
  To: Paul Moore; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

On Fri, 28 Jan 2011, Paul Moore wrote:

> 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.

Perhaps something is needed, but I don't see how the current code can 
work.  The call is genlmsg_cancel(cb_arg->skb, NULL) in each case.

The definition of genlmsg_cancel is:

static inline void genlmsg_cancel(struct sk_buff *skb, void *hdr)
{
	nlmsg_cancel(skb, hdr - GENL_HDRLEN - NLMSG_HDRLEN);
}

Now the second argument to nlmsg_cancel is essentially a negative integer 
(or a very large pointer).

nlmsg_cancel will call nlmsg_trim, which is defined as follows:

static inline void nlmsg_trim(struct sk_buff *skb, const void *mark)
{
	if (mark)
		skb_trim(skb, (unsigned char *) mark - skb->data);
}

I guess that the subtraction is going to result in an even larger negative 
number.  The whole process is likely to end in doing nothing in the 
definition of skb_trim, which is as follows:

void skb_trim(struct sk_buff *skb, unsigned int len)
{
	if (skb->len > len)
		__skb_trim(skb, len);
}

since the result of casting a negative number to unsigned is likely to be 
larger than skb->len.


> > ---
> >  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
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 14:58     ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-01-28 14:58 UTC (permalink / raw)
  To: Paul Moore; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

On Fri, 28 Jan 2011, Paul Moore wrote:

> 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.

Perhaps something is needed, but I don't see how the current code can 
work.  The call is genlmsg_cancel(cb_arg->skb, NULL) in each case.

The definition of genlmsg_cancel is:

static inline void genlmsg_cancel(struct sk_buff *skb, void *hdr)
{
	nlmsg_cancel(skb, hdr - GENL_HDRLEN - NLMSG_HDRLEN);
}

Now the second argument to nlmsg_cancel is essentially a negative integer 
(or a very large pointer).

nlmsg_cancel will call nlmsg_trim, which is defined as follows:

static inline void nlmsg_trim(struct sk_buff *skb, const void *mark)
{
	if (mark)
		skb_trim(skb, (unsigned char *) mark - skb->data);
}

I guess that the subtraction is going to result in an even larger negative 
number.  The whole process is likely to end in doing nothing in the 
definition of skb_trim, which is as follows:

void skb_trim(struct sk_buff *skb, unsigned int len)
{
	if (skb->len > len)
		__skb_trim(skb, len);
}

since the result of casting a negative number to unsigned is likely to be 
larger than skb->len.


> > ---
> >  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
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
  2011-01-28 14:58     ` Julia Lawall
@ 2011-01-28 15:23       ` Paul Moore
  -1 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2011-01-28 15:23 UTC (permalink / raw)
  To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

On Fri, 2011-01-28 at 15:58 +0100, Julia Lawall wrote:
> On Fri, 28 Jan 2011, Paul Moore wrote:
> 
> > 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.
> 
> Perhaps something is needed, but I don't see how the current code can 
> work.  The call is genlmsg_cancel(cb_arg->skb, NULL) in each case.

Ah yes, you're right.  You will have to forgive me as it has been quite
a while since I have looked at NetLabel's netlink code.

You also might consider putting a NULL check in genlmsg_cancel() similar
to the check nlmsg_trim(); that seems like a worthwhile addition.

> The definition of genlmsg_cancel is:
> 
> static inline void genlmsg_cancel(struct sk_buff *skb, void *hdr)
> {
> 	nlmsg_cancel(skb, hdr - GENL_HDRLEN - NLMSG_HDRLEN);
> }
> 
> Now the second argument to nlmsg_cancel is essentially a negative integer 
> (or a very large pointer).
> 
> nlmsg_cancel will call nlmsg_trim, which is defined as follows:
> 
> static inline void nlmsg_trim(struct sk_buff *skb, const void *mark)
> {
> 	if (mark)
> 		skb_trim(skb, (unsigned char *) mark - skb->data);
> }
> 
> I guess that the subtraction is going to result in an even larger negative 
> number.  The whole process is likely to end in doing nothing in the 
> definition of skb_trim, which is as follows:
> 
> void skb_trim(struct sk_buff *skb, unsigned int len)
> {
> 	if (skb->len > len)
> 		__skb_trim(skb, len);
> }
> 
> since the result of casting a negative number to unsigned is likely to be 
> larger than skb->len.
> 
> 
> > > ---
> > >  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
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

-- 
paul moore
linux @ hp



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 15:23       ` Paul Moore
  0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2011-01-28 15:23 UTC (permalink / raw)
  To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

On Fri, 2011-01-28 at 15:58 +0100, Julia Lawall wrote:
> On Fri, 28 Jan 2011, Paul Moore wrote:
> 
> > 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.
> 
> Perhaps something is needed, but I don't see how the current code can 
> work.  The call is genlmsg_cancel(cb_arg->skb, NULL) in each case.

Ah yes, you're right.  You will have to forgive me as it has been quite
a while since I have looked at NetLabel's netlink code.

You also might consider putting a NULL check in genlmsg_cancel() similar
to the check nlmsg_trim(); that seems like a worthwhile addition.

> The definition of genlmsg_cancel is:
> 
> static inline void genlmsg_cancel(struct sk_buff *skb, void *hdr)
> {
> 	nlmsg_cancel(skb, hdr - GENL_HDRLEN - NLMSG_HDRLEN);
> }
> 
> Now the second argument to nlmsg_cancel is essentially a negative integer 
> (or a very large pointer).
> 
> nlmsg_cancel will call nlmsg_trim, which is defined as follows:
> 
> static inline void nlmsg_trim(struct sk_buff *skb, const void *mark)
> {
> 	if (mark)
> 		skb_trim(skb, (unsigned char *) mark - skb->data);
> }
> 
> I guess that the subtraction is going to result in an even larger negative 
> number.  The whole process is likely to end in doing nothing in the 
> definition of skb_trim, which is as follows:
> 
> void skb_trim(struct sk_buff *skb, unsigned int len)
> {
> 	if (skb->len > len)
> 		__skb_trim(skb, len);
> }
> 
> since the result of casting a negative number to unsigned is likely to be 
> larger than skb->len.
> 
> 
> > > ---
> > >  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
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

-- 
paul moore
linux @ hp



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
  2011-01-28 15:23       ` Paul Moore
@ 2011-01-28 15:29         ` Julia Lawall
  -1 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-01-28 15:29 UTC (permalink / raw)
  To: Paul Moore; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

On Fri, 28 Jan 2011, Paul Moore wrote:

> On Fri, 2011-01-28 at 15:58 +0100, Julia Lawall wrote:
> > On Fri, 28 Jan 2011, Paul Moore wrote:
> > 
> > > 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.
> > 
> > Perhaps something is needed, but I don't see how the current code can 
> > work.  The call is genlmsg_cancel(cb_arg->skb, NULL) in each case.
> 
> Ah yes, you're right.  You will have to forgive me as it has been quite
> a while since I have looked at NetLabel's netlink code.
> 
> You also might consider putting a NULL check in genlmsg_cancel() similar
> to the check nlmsg_trim(); that seems like a worthwhile addition.

OK, I'll do that.

julia

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 15:29         ` Julia Lawall
  0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2011-01-28 15:29 UTC (permalink / raw)
  To: Paul Moore; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

On Fri, 28 Jan 2011, Paul Moore wrote:

> On Fri, 2011-01-28 at 15:58 +0100, Julia Lawall wrote:
> > On Fri, 28 Jan 2011, Paul Moore wrote:
> > 
> > > 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.
> > 
> > Perhaps something is needed, but I don't see how the current code can 
> > work.  The call is genlmsg_cancel(cb_arg->skb, NULL) in each case.
> 
> Ah yes, you're right.  You will have to forgive me as it has been quite
> a while since I have looked at NetLabel's netlink code.
> 
> You also might consider putting a NULL check in genlmsg_cancel() similar
> to the check nlmsg_trim(); that seems like a worthwhile addition.

OK, I'll do that.

julia

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
  2011-01-28 14:17 ` Julia Lawall
@ 2011-01-28 15:46   ` Paul Moore
  -1 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2011-01-28 15:46 UTC (permalink / raw)
  To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

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>

Acked-by: Paul Moore <paul.moore@hp.com>

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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel
@ 2011-01-28 15:46   ` Paul Moore
  0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2011-01-28 15:46 UTC (permalink / raw)
  To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, netdev, linux-kernel

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>

Acked-by: Paul Moore <paul.moore@hp.com>

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



^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-01-28 15:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-28 13:58 [PATCH 2/2] net/netlabel: Avoid call to genlmsg_cancel Julia Lawall
2011-01-28 14:17 ` Julia Lawall
2011-01-28 14:36 ` Paul Moore
2011-01-28 14:36   ` Paul Moore
2011-01-28 14:58   ` Julia Lawall
2011-01-28 14:58     ` Julia Lawall
2011-01-28 15:23     ` Paul Moore
2011-01-28 15:23       ` Paul Moore
2011-01-28 15:29       ` Julia Lawall
2011-01-28 15:29         ` Julia Lawall
2011-01-28 15:46 ` Paul Moore
2011-01-28 15:46   ` Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.