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