Re: [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv

[Alexander Aring <alex.aring@gmail.com>]

On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
> Hi Alex,
> On 16/09/14 12:36, Alexander Aring wrote:
> > On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
> >> Currently there are a number of error paths in the lowpan_rcv function that
> >> free the skb before returning, the patch simplifies the receive path by
> >> ensuring that the skb is only freed from this function.
> >>
> >> Passing the skb from 6lowpan up to the higher layers is not a
> >> function of IPHC.  By moving it out of IPHC we also remove the
> >> need to support error code returns with NET_RX codes.
> >> It also makes the lowpan_rcv function more extendable as we
> >> can support more compression schemes.
> >>
> >> With the above 2 lowpan_rcv is refacored so eliminate incorrect return values.
> >>
> >> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> >> ---
> >>  include/net/6lowpan.h         |  9 +++--
> >>  net/6lowpan/iphc.c            | 88 +++++++++++++++++--------------------------
> >>  net/bluetooth/6lowpan.c       | 38 ++++++++++---------
> >>  net/ieee802154/6lowpan_rtnl.c | 61 ++++++++++++++++--------------
> >>  4 files changed, 94 insertions(+), 102 deletions(-)
> >>
> >> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> >> index d184df1..c28fadb 100644
> >> --- a/include/net/6lowpan.h
> >> +++ b/include/net/6lowpan.h
> > ...
> >> -static int process_data(struct sk_buff *skb, struct net_device *netdev,
> >> -			struct l2cap_chan *chan)
> >> +static struct sk_buff *
> >> +process_data(struct sk_buff *skb, struct net_device *netdev,
> >> +	     struct l2cap_chan *chan)
> >>  {
> >>  	const u8 *saddr, *daddr;
> >>  	u8 iphc0, iphc1;
> >> @@ -230,36 +231,31 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
> >>  	peer = peer_lookup_chan(dev, chan);
> >>  	read_unlock_irqrestore(&devices_lock, flags);
> >>  	if (!peer)
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	saddr = peer->eui64_addr;
> >>  	daddr = dev->netdev->dev_addr;
> >>  
> >>  	/* at least two bytes will be used for the encoding */
> >>  	if (skb->len < 2)
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	return lowpan_process_data(skb, netdev,
> >>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> >>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> >> -				   iphc0, iphc1, give_skb_to_upper);
> >> -
> >> -drop:
> >> -	kfree_skb(skb);
> >> -	return -EINVAL;
> >> +				   iphc0, iphc1);
> >>  }
> >>  
> >>  static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >>  		    struct l2cap_chan *chan)
> >>  {
> >>  	struct sk_buff *local_skb;
> >> -	int ret;
> >>  
> >>  	if (!netif_running(dev))
> >>  		goto drop;
> >> @@ -280,9 +276,6 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >>  		local_skb->protocol = htons(ETH_P_IPV6);
> >>  		local_skb->pkt_type = PACKET_HOST;
> >>  
> >> -		skb_reset_network_header(local_skb);
> >> -		skb_set_transport_header(local_skb, sizeof(struct ipv6hdr));
> >> -
> >>  		if (give_skb_to_upper(local_skb, dev) != NET_RX_SUCCESS) {
> >>  			kfree_skb(local_skb);
> >>  			goto drop;
> >> @@ -300,9 +293,20 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >>  			if (!local_skb)
> >>  				goto drop;
> >>  
> >> -			ret = process_data(local_skb, dev, chan);
> >> -			if (ret != NET_RX_SUCCESS)
> >> +			skb = process_data(local_skb, dev, chan);
> >> +			if (IS_ERR(skb)) {
> >> +				kfree_skb(local_skb);
> >>  				goto drop;
> >> +			}
> >> +
> >> +			local_skb->protocol = htons(ETH_P_IPV6);
> >> +			local_skb->pkt_type = PACKET_HOST;
> > this is the wrong skb here, the new one is skb. I don't know maybe there
> > is some optimization to call skb = process_data(skb, ...);

and this also smells like side effects for me, because we have the
local_skb which is sometimes freed inside of lowpan_process_data and
returning skb. Then we don't know which we should kfree_skb now, the skb
or local_skb now. Need to thing more about this to offer some solution,
somebody agree here with me?

- Alex