Hi Martin,
On Mon, Oct 20, 2014 at 10:35:25PM +0100, Martin Townsend wrote:
> 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.
> >
<qoute>
static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
{
struct sk_buff *skb_cp;
skb_cp = skb_copy(skb, GFP_ATOMIC);
if (!skb_cp)
return -ENOMEM;
Returning errno here.
return netif_rx(skb_cp);
Returning NET_RX_FOO here. Here should be the same behaviour like below.
Only without a loop. Because the skb_share_check and only one lowpan
interface, I think we don't need the skb_copy here. This is out of scope
in this patch.
}
</qoute>
> >>+ 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.
mhh, okay.
> 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??
>
static int lowpan_give_skb_to_devices(struct sk_buff *skb,
struct net_device *dev)
{
struct lowpan_dev_record *entry;
struct sk_buff *skb_cp;
int stat = NET_RX_SUCCESS;
rcu_read_lock();
list_for_each_entry_rcu(entry, &lowpan_devices, list)
if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
skb_cp = skb_copy(skb, GFP_ATOMIC);
if (!skb_cp) {
kfree_skb(skb);
rcu_read_unlock();
return NET_RX_DROP;
}
skb_cp->dev = entry->ldev;
stat = netif_rx(skb_cp);
if (stat == NET_RX_DROP)
break;
}
rcu_read_unlock();
consume_skb(skb);
return stat;
}
We don't need no explicit run of kfree_skb or consume_skb here. in
this case "skb" should only a skb where we create copies from. After
that we don't dropping the skb. Okay, we only drop it if allocation
failed. I this okay now or I overlooked something here?
- 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
