Re: [Linux-zigbee-devel] [PATCH 1/2] Remove dev parameter from skb_delivery_cb in 6lowpan.

Re: [Linux-zigbee-devel] [PATCH 1/2] Remove dev parameter from skb_delivery_cb in 6lowpan.

[Alexander Aring]

Hi Martin,

this patch looks good, simple rebase it on bluetooth-next.

You could do:

git remote add bluetooth-next
git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git

git checkout bluetooth-next/master

then git cherry-pick $COMMIT_ID

to get the necessary commits from other branches, it should fail but you
need to solve the conflicts.

On Wed, Jul 30, 2014 at 04:25:22PM +0100, Martin Townsend wrote:
> This parameter is never used by any functions that are passed to
> lowpan_process_data which uses this callback.
> 
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  include/net/6lowpan.h         | 2 +-
>  net/6lowpan/iphc.c            | 2 +-
>  net/bluetooth/6lowpan.c       | 4 ++--
>  net/ieee802154/6lowpan_rtnl.c | 5 ++---
>  4 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index 79b530f..995cce86 100644
> --- a/include/net/6lowpan.h
> +++ b/include/net/6lowpan.h
> @@ -422,7 +422,7 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
>  	return skb->len + uncomp_header - ret;
>  }
>  
> -typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
> +typedef int (*skb_delivery_cb)(struct sk_buff *skb);
>  
>  int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index e82c9cc..b4bb27c 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -195,7 +195,7 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>  	raw_dump_table(__func__, "raw skb data dump before receiving",
>  		       new->data, new->len);
>  
> -	stat = deliver_skb(new, dev);
> +	stat = deliver_skb(new);
>  
>  	kfree_skb(new);
>  
I know you didn't change it and you should do this in a seperate
patch, but this should be consume_skb or dev_kfree_skb. We don't drop
the skb afterwards here with failure. The complete file have several
places like this and use kfree_skb here. We should do this in one of the
next patches.

- Alex

Re: [Linux-zigbee-devel] [PATCH 2/2] Change lowpan_rcv so skb is freed within function and fix return values.

[Alexander Aring]

Hi Martin,

I agree about the issue, we talked a lot time before about this. But I
would not complete remove the errno numbers.

The basic issue is that that mixed function can return errno or
NET_RX_DROP/NET_RX_SUCCESS and we need to decide if we use
NET_RX_DROP/NET_RX_SUCCESS only or using errno's but then we need a
conversion of the errno to NET_RX_DROP at the end of receive call, If
a errno was returned.

If all functions return "-1" now, it's hard to debug what could be
failed, when we add some debug prints in this file.

On Wed, Jul 30, 2014 at 04:25:23PM +0100, Martin Townsend wrote:
> Currently it is up to the functions below lowpan_rcv to free the skb on error
> conditions.  This patch now removes all the UAPI error codes and process data
> now returns -1 if there is a problem.  In this scenario lowpan_rcv will free
> the skb and return NET_RX_DROP.  This also fixes the problem where
> NET_RX_SUCCESS is returned on error
> 
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  include/net/6lowpan.h         |  2 +-
>  net/6lowpan/iphc.c            | 35 ++++++++++++++++++-----------------
>  net/bluetooth/6lowpan.c       | 17 ++++++++---------
>  net/ieee802154/6lowpan_rtnl.c | 39 +++++++++++++++++++--------------------
>  4 files changed, 46 insertions(+), 47 deletions(-)
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index 995cce86..561defe 100644
> --- a/include/net/6lowpan.h
> +++ b/include/net/6lowpan.h
> @@ -424,7 +424,7 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
>  
>  typedef int (*skb_delivery_cb)(struct sk_buff *skb);
>  
> -int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> +int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
>  		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>  		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>  		u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index b4bb27c..61b5206 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -119,17 +119,17 @@ static int uncompress_addr(struct sk_buff *skb,
>  			break;
>  		default:
>  			pr_debug("Invalid addr_type set\n");
> -			return -EINVAL;
> +			return -1;
>  		}
>  		break;
>  	default:
>  		pr_debug("Invalid address mode value: 0x%x\n", address_mode);
> -		return -EINVAL;
> +		return -1;
>  	}
>  
>  	if (fail) {
>  		pr_debug("Failed to fetch skb data\n");
> -		return -EIO;
> +		return -1;
>  	}
>  
>  	raw_dump_inline(NULL, "Reconstructed ipv6 addr is",
> @@ -158,10 +158,10 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
>  	case LOWPAN_IPHC_ADDR_03:
>  		/* TODO */
>  		netdev_warn(skb->dev, "SAM value 0x%x not supported\n", sam);
> -		return -EINVAL;
> +		return -1;
>  	default:
>  		pr_debug("Invalid sam value: 0x%x\n", sam);
> -		return -EINVAL;
> +		return -1;
>  	}
>  
>  	raw_dump_inline(NULL,
> @@ -179,10 +179,10 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>  
>  	new = skb_copy_expand(skb, sizeof(struct ipv6hdr),
>  			      skb_tailroom(skb), GFP_ATOMIC);
> -	kfree_skb(skb);
> -
>  	if (!new)
> -		return -ENOMEM;
> +		return -1;
> +
> +	kfree_skb(skb);

This leaks memory here, move kfree_skb above the condition after the
skb_copy_expand. This should be also a consume_skb or dev_kfree_skb, we
should not mix these function in this file and use only one of them.

We need another patch for this.

>  
>  	skb_push(new, sizeof(struct ipv6hdr));
>  	skb_reset_network_header(new);
> @@ -196,6 +196,8 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>  		       new->data, new->len);
>  
>  	stat = deliver_skb(new);
> +	if (stat == -1)

keep all existing errnos and look for function which are called but
returning NET_RX_DROP. Then check for if (stat < 0) to indicate a error.

- Alex

Re: [Linux-zigbee-devel] [PATCH 2/2] Change lowpan_rcv so skb is freed within function and fix return values.

[Alexander Aring]

On Thu, Jul 31, 2014 at 07:54:27AM +0200, Alexander Aring wrote:
...
> >  
> >  	raw_dump_inline(NULL,
> > @@ -179,10 +179,10 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
> >  
> >  	new = skb_copy_expand(skb, sizeof(struct ipv6hdr),
> >  			      skb_tailroom(skb), GFP_ATOMIC);
> > -	kfree_skb(skb);
> > -
> >  	if (!new)
> > -		return -ENOMEM;
> > +		return -1;
> > +
> > +	kfree_skb(skb);
> 
> This leaks memory here, move kfree_skb above the condition after the
> skb_copy_expand. This should be also a consume_skb or dev_kfree_skb, we
> should not mix these function in this file and use only one of them.
> 
> We need another patch for this.
>

Ah, no there is no memory leaking sorry. You removed also the
kfree_skb(skb) in the drop label and we do this at one place to detect if
an error occur.

Maybe we can return the errno at the drop label and set err = -ENOMEM
and do a goto drop.

- Alex