On 10/07/14 12:47, Alexander Aring wrote:
> Hi Martin,
>
> yes you are right here, we talking about this some months ago and I
> simple not having time to do anything for that.
I remember but we are seeing our devices panic due to this. We have devices
that aren't linux based that are used in point to point testing that send out
packets with PRN sequences. Some of these packets are making there way into
the 802.15.4/6lowpan stack. At least it's a good test of all the error paths :)
>
> There was also some 6lowpan cleanup patches...
>
> I preparing now to have a "single point of pull" for david miller and
> send david pull-request for all ieee802154 subsystem changes.
>
> This change isn't done yet. I wait for the mailinglist on
> vger.kernel.org...
>
> I will write a summary for all changes in next days.
>
> Nevertheless I can apply your changes to the wpan tree or we send it now
> to net-next.
As this really only effects us I don't mind applying to wpan tree.
> - Alex
>
> On Thu, Jul 10, 2014 at 11:59:01AM +0100, Martin Townsend wrote:
>> The functions lowpan_give_skb_to_devices and process_data can return a
>> negative error code as well as NET_RX_XXX values. Before this patch if
>> either function returned an error code it would return NET_RX_SUCCESS even
>> though the skb would have been freed.
>>
> The commit message should not longer than 80 chars. Also you need to tag
> your patch on which branch this based. Your patch is something for
> stable because it's already in some of the stable tree's.
>
> [PATCH net] 6lowpan: Ensure lowpan_rcv returns correct value
>
> Or simple run 'git format-patch -s -1 --subject-prefix="PATCH net"'.
>
> But please rebase your code then on the net tree, not net-next.
Will do, thanks for the tip.
> - Alex
>
>> Signed-off-by: Martin Townsend <martin.towns...@xsilon.com>
>> ---
>> net/ieee802154/6lowpan_rtnl.c | 21 ++++++---------------
>> 1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
>> index 0f5a69e..a970c8c 100644
>> --- a/net/ieee802154/6lowpan_rtnl.c
>> +++ b/net/ieee802154/6lowpan_rtnl.c
>> @@ -448,7 +448,6 @@ static int lowpan_rcv(struct sk_buff *skb, struct
>> net_device *dev,
>> struct packet_type *pt, struct net_device *orig_dev)
>> {
>> struct ieee802154_hdr hdr;
>> - int ret;
>>
>> skb = skb_share_check(skb, GFP_ATOMIC);
>> if (!skb)
>> @@ -471,31 +470,23 @@ static int lowpan_rcv(struct sk_buff *skb, struct
>> net_device *dev,
>> /* Pull off the 1-byte of 6lowpan header. */
>> skb_pull(skb, 1);
>>
>> - ret = lowpan_give_skb_to_devices(skb, NULL);
>> - if (ret == NET_RX_DROP)
>> + if (lowpan_give_skb_to_devices(skb, NULL) != NET_RX_SUCCESS)
>> goto drop;
>> } else {
>> switch (skb->data[0] & 0xe0) {
>> case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
>> - ret = process_data(skb, &hdr);
>> - if (ret == NET_RX_DROP)
>> + if (process_data(skb, &hdr) != NET_RX_SUCCESS)
>> goto drop;
>> break;
>> case LOWPAN_DISPATCH_FRAG1: /* first fragment header */
>> - ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
> For example this. lowpan_frag_rcv can also return -1 on error and
> return NET_RX_SUCCESS, which is wrong... that's why we need to evalute
> on ret some or later.
>
> Something like that:
>
> if (ret < 0)
> goto drop;
>
> but we should go to drop_skb;
>
> and remove the kfree_skb from reassembly code. This avoids much
> side-effects. But we need to handle that in process_data also. kfree_skb
> is called in many of calling functions but we need a error handling
> which free's at the end of call stack of functions... it's a little bit
> more complex to handle it. :-(
Agreed, I did try and follow the code but found there are so many paths so I
fixed it in lowpan_rcv.
>
>> - if (ret == 1) {
>> - ret = process_data(skb, &hdr);
>> - if (ret == NET_RX_DROP)
>> + if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1) == 1)
>> + if (process_data(skb, &hdr) != NET_RX_SUCCESS)
>> goto drop;
>> - }
>> break;
>> case LOWPAN_DISPATCH_FRAGN: /* next fragments headers */
>> - ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
>> - if (ret == 1) {
>> - ret = process_data(skb, &hdr);
>> - if (ret == NET_RX_DROP)
>> + if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN) == 1)
>> + if (process_data(skb, &hdr) != NET_RX_SUCCESS)
> We currently return -EINVAL can you change the errno check to
>
> if (ret < 0)
> goto drop;
>
> instead of != NET_RX_SUCCESS please and don't call functions inside the
> condition.
can do, just trying to keep the code size down. personally I would have also
combined both frag cases to avoid code duplication.
>
> - Alex
- Martin
------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
