Return-Path: Date: Tue, 16 Sep 2014 13:48:01 +0200 From: Alexander Aring To: Martin Townsend Cc: Martin Townsend , linux-zigbee-devel@lists.sourceforge.net, linux-bluetooth@vger.kernel.org, linux-wpan@vger.kernel.org, marcel@holtmann.org Subject: Re: [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv Message-ID: <20140916114759.GD4969@omega> References: <1410865319-16310-1-git-send-email-martin.townsend@xsilon.com> <1410865319-16310-2-git-send-email-martin.townsend@xsilon.com> <20140916113614.GC4969@omega> <5418215F.5050308@xsilon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <5418215F.5050308@xsilon.com> List-ID: 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 > >> --- > >> 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