Hi Alex,
On 20/10/14 20:18, Alexander Aring wrote:
Hi Martin,
On Mon, Oct 20, 2014 at 03:39:48PM +0100, Martin Townsend wrote:
Separating skb delivery from decompression ensures that we can support further
decompression schemes and removes the mixed return value of error codes with
NET_RX_FOO.
Signed-off-by: Martin Townsend <mtownsend1973@xxxxxxxxx>
Signed-off-by: Martin Townsend <martin.townsend@xxxxxxxxxx>
All your patches have two different "Signed-off-by". Please use only one
here.
I think I know how this occurred, 2 difference computers with 2
different git configs :) I'll fix this in the next series.
---
include/net/6lowpan.h | 4 +---
net/6lowpan/iphc.c | 32 ++++++--------------------------
net/bluetooth/6lowpan.c | 12 +++++++++++-
net/ieee802154/6lowpan_rtnl.c | 26 ++++++++++++++------------
4 files changed, 32 insertions(+), 42 deletions(-)
...
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 6c5c2ef..03787e0 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -290,7 +290,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
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);
+ iphc0, iphc1);
drop:
kfree_skb(skb);
@@ -350,6 +350,16 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
if (ret != NET_RX_SUCCESS)
goto drop;
+ local_skb->protocol = htons(ETH_P_IPV6);
+ local_skb->pkt_type = PACKET_HOST;
+ local_skb->dev = dev;
+
+ if (give_skb_to_upper(local_skb, dev)
+ != NET_RX_SUCCESS) {
There is still a NET_RX_FOO and errno conversion in function
"give_skb_to_upper". Please check that, maybe introduce a new patch for
this one.
I thought give_skb_to_upper only returned NET_RX_FOO return values? I'll
double check and fix.
+ kfree_skb(local_skb);
+ goto drop;
+ }
+
dev->stats.rx_bytes += skb->len;
dev->stats.rx_packets++;
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 0c1a49b..898d317 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -146,8 +146,9 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
skb_cp = skb_copy(skb, GFP_ATOMIC);
if (!skb_cp) {
- stat = -ENOMEM;
- break;
+ kfree_skb(skb);
+ rcu_read_unlock();
+ return NET_RX_DROP;
}
skb_cp->dev = entry->ldev;
@@ -155,6 +156,11 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
}
rcu_read_unlock();
+ if (stat == NET_RX_SUCCESS)
+ consume_skb(skb);
+ else
+ kfree_skb(skb);
+
You will not see it because somebody forgot brackets in the above
"list_for_each_entry_rcu" loop. But variable "stat" in this case is only
for the last entry of netif_rx call.
This is a bug that's probably outside the scope of this patch.
Also if netif_rx returns NET_RX_DROP, which is the else branch in this
case. In case of netif_rx the skb will be freed somewhere else.
Calltrace:
- netif_rx
- netif_rx_internal
- enqueue_to_backlog
- kfree_skb(skb);
- return NET_RX_DROP;
The copy will be freed not the skb that is passed to the function.
skb_cp = skb_copy(skb, GFP_ATOMIC);
....
stat = netif_rx(skb_cp);
What to do with stat that is set multiple times in a loop. We could
call skb_consume if stat is always NET_RX_SUCCESS or call skb_consume if
stat is at least NET_RX_SUCCESS once, or maybe remove the loop further
out the call chain ... if possible??
- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
- Martin.
--
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
