Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Martin,
On Mon, Sep 15, 2014 at 03:09:54PM +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.towns...@xsilon.com
---
include/net/6lowpan.h | 4 +--
net/6lowpan/iphc.c| 74
++-
net/bluetooth/6lowpan.c | 26 +--
net/ieee802154/6lowpan_rtnl.c | 55
4 files changed, 83 insertions(+), 76 deletions(-)
diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..450eaaf 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -374,10 +374,10 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16
*dgram_offset)
typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+ u8 iphc0, u8 iphc1);
int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
unsigned short type, const void *_daddr,
const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..d51e8bb 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,37 +171,6 @@ static int uncompress_context_based_src_addr(struct
sk_buff *skb,
return 0;
}
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
-struct net_device *dev, skb_delivery_cb deliver_skb)
-{
- struct sk_buff *new;
- int stat;
-
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
-
- new-protocol = htons(ETH_P_IPV6);
- new-pkt_type = PACKET_HOST;
- new-dev = dev;
-
- raw_dump_table(__func__, raw skb data dump before receiving,
-new-data, new-len);
-
- stat = deliver_skb(new, dev);
-
- kfree_skb(new);
-
- return stat;
-}
-
/* Uncompress function for multicast destination address,
* when M bit is set.
*/
@@ -332,14 +301,15 @@ err:
/* TTL uncompression values */
static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
const u8 *saddr, const u8 saddr_type, const u8
saddr_len,
const u8 *daddr, const u8 daddr_type, const u8
daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+ u8 iphc0, u8 iphc1)
{
struct ipv6hdr hdr = {};
u8 tmp, num_context = 0;
- int err;
+ int err = -EINVAL;
+ struct sk_buff *skb = *skb_inout;
raw_dump_table(__func__, raw skb data dump uncompressed,
skb-data, skb-len);
@@ -460,7 +430,6 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
/* UDP data uncompression */
if (iphc0 LOWPAN_IPHC_NH_C) {
struct udphdr uh;
- struct sk_buff *new;
if (uncompress_udp_header(skb, uh))
goto drop;
@@ -468,14 +437,15 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
/* replace the compressed UDP head by the uncompressed UDP
* header
*/
- new = skb_copy_expand(skb, sizeof(struct udphdr),
+ skb = skb_copy_expand(skb, sizeof(struct udphdr),
skb_tailroom(skb), GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
+ if (!skb) {
+ err = -ENOMEM;
+ goto drop;
+ }
- skb = new;
+ kfree_skb(*skb_inout);
+ *skb_inout = skb;
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Martin, On ma, 2014-09-15 at 15:09 +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. I like the idea that we get rid of the callback function. We could probably refactor the patch a bit further thou as the lowpan_process_data() could return the skb directly instead of being passed as a parameter. In the caller we could use the IS_ERR() macro to check if the returned value is an error or a real pointer. Cheers, Jukka -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 10:04:57AM +0300, Jukka Rissanen wrote: Hi Martin, On ma, 2014-09-15 at 15:09 +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. I like the idea that we get rid of the callback function. We could probably refactor the patch a bit further thou as the lowpan_process_data() could return the skb directly instead of being passed as a parameter. In the caller we could use the IS_ERR() macro to check if the returned value is an error or a real pointer. ack, then we can also drop the **inout_skb thing. - Alex -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On 16/09/14 07:57, Alexander Aring wrote:
Hi Martin,
On Mon, Sep 15, 2014 at 03:09:54PM +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.towns...@xsilon.com
---
include/net/6lowpan.h | 4 +--
net/6lowpan/iphc.c| 74
++-
net/bluetooth/6lowpan.c | 26 +--
net/ieee802154/6lowpan_rtnl.c | 55
4 files changed, 83 insertions(+), 76 deletions(-)
diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..450eaaf 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -374,10 +374,10 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16
*dgram_offset)
typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+u8 iphc0, u8 iphc1);
int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
unsigned short type, const void *_daddr,
const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..d51e8bb 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,37 +171,6 @@ static int uncompress_context_based_src_addr(struct
sk_buff *skb,
return 0;
}
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
- struct net_device *dev, skb_delivery_cb deliver_skb)
-{
-struct sk_buff *new;
-int stat;
-
-new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
-kfree_skb(skb);
-
-if (!new)
-return -ENOMEM;
-
-skb_push(new, sizeof(struct ipv6hdr));
-skb_reset_network_header(new);
-skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
-
-new-protocol = htons(ETH_P_IPV6);
-new-pkt_type = PACKET_HOST;
-new-dev = dev;
-
-raw_dump_table(__func__, raw skb data dump before receiving,
- new-data, new-len);
-
-stat = deliver_skb(new, dev);
-
-kfree_skb(new);
-
-return stat;
-}
-
/* Uncompress function for multicast destination address,
* when M bit is set.
*/
@@ -332,14 +301,15 @@ err:
/* TTL uncompression values */
static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
const u8 *saddr, const u8 saddr_type, const u8
saddr_len,
const u8 *daddr, const u8 daddr_type, const u8
daddr_len,
-u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+u8 iphc0, u8 iphc1)
{
struct ipv6hdr hdr = {};
u8 tmp, num_context = 0;
-int err;
+int err = -EINVAL;
+struct sk_buff *skb = *skb_inout;
raw_dump_table(__func__, raw skb data dump uncompressed,
skb-data, skb-len);
@@ -460,7 +430,6 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
/* UDP data uncompression */
if (iphc0 LOWPAN_IPHC_NH_C) {
struct udphdr uh;
-struct sk_buff *new;
if (uncompress_udp_header(skb, uh))
goto drop;
@@ -468,14 +437,15 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
/* replace the compressed UDP head by the uncompressed UDP
* header
*/
-new = skb_copy_expand(skb, sizeof(struct udphdr),
+skb = skb_copy_expand(skb, sizeof(struct udphdr),
skb_tailroom(skb), GFP_ATOMIC);
-kfree_skb(skb);
-
-if (!new)
-return -ENOMEM;
+if (!skb) {
+err = -ENOMEM;
+goto drop;
+}
-skb = new;
+kfree_skb(*skb_inout);
+*skb_inout = skb;
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 09:28:10AM +0100, Martin Townsend wrote:
Hi Alex,
On 16/09/14 07:57, Alexander Aring wrote:
Hi Martin,
On Mon, Sep 15, 2014 at 03:09:54PM +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.towns...@xsilon.com
---
include/net/6lowpan.h | 4 +--
net/6lowpan/iphc.c| 74
++-
net/bluetooth/6lowpan.c | 26 +--
net/ieee802154/6lowpan_rtnl.c | 55
4 files changed, 83 insertions(+), 76 deletions(-)
diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..450eaaf 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -374,10 +374,10 @@ lowpan_uncompress_size(const struct sk_buff *skb,
u16 *dgram_offset)
typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device
*dev);
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device
*dev,
const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+ u8 iphc0, u8 iphc1);
int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
unsigned short type, const void *_daddr,
const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..d51e8bb 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,37 +171,6 @@ static int uncompress_context_based_src_addr(struct
sk_buff *skb,
return 0;
}
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
- struct net_device *dev, skb_delivery_cb deliver_skb)
-{
- struct sk_buff *new;
- int stat;
-
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
-GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
-
- new-protocol = htons(ETH_P_IPV6);
- new-pkt_type = PACKET_HOST;
- new-dev = dev;
-
- raw_dump_table(__func__, raw skb data dump before receiving,
- new-data, new-len);
-
- stat = deliver_skb(new, dev);
-
- kfree_skb(new);
-
- return stat;
-}
-
/* Uncompress function for multicast destination address,
* when M bit is set.
*/
@@ -332,14 +301,15 @@ err:
/* TTL uncompression values */
static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device
*dev,
const u8 *saddr, const u8 saddr_type, const u8
saddr_len,
const u8 *daddr, const u8 daddr_type, const u8
daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+ u8 iphc0, u8 iphc1)
{
struct ipv6hdr hdr = {};
u8 tmp, num_context = 0;
- int err;
+ int err = -EINVAL;
+ struct sk_buff *skb = *skb_inout;
raw_dump_table(__func__, raw skb data dump uncompressed,
skb-data, skb-len);
@@ -460,7 +430,6 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
/* UDP data uncompression */
if (iphc0 LOWPAN_IPHC_NH_C) {
struct udphdr uh;
- struct sk_buff *new;
if (uncompress_udp_header(skb, uh))
goto drop;
@@ -468,14 +437,15 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
/* replace the compressed UDP head by the uncompressed UDP
* header
*/
- new = skb_copy_expand(skb, sizeof(struct udphdr),
+ skb = skb_copy_expand(skb, sizeof(struct udphdr),
skb_tailroom(skb), GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
+ if (!skb) {
+ err = -ENOMEM;
+ goto drop;
+ }
-
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 11:06:08AM +0200, Alexander Aring wrote: ... Are you saying that skb-len needs setting here? or just that to do on the fly decompression it's required? and I was wrong here, I mean we need the IPv6 header payload need to set according skb-len - sizeof(...ipv6hdr). that's currently setted while uncompression, but for our use case with fragmentation while receiving FRAG1 skb-len is wrong. btw. I also detected right now that makes also trouble with next header compression payload size attributes. But this is a complete other issue. We need make this as next step when we insert next header framework. Otherwise we can't uncompress on the fly while receiving FRAG1, but we need to handle this in that way to remove the ugly workaround solution. Nobody says that this would be easy. - Alex -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On the lowpan_give_skb_to_devices change.
As we are iterating over a list of lowpan_devices and could potentially copy
the skb more than once, what happens if the first device returns NET_RX_DROP
and then the second time it return NET_RX_SUCCESS? The stat variable is
overwritten so stat only ever reflects the return value of netif_rx for the
last device?
Maybe it's better to completely remove the if else at the end and always
consume the skb? For the case whereskb_copy fails then we should kfree_skb,
e.g.
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);
}
rcu_read_unlock();
consume_skb(skb);
return stat;
}
what are your thoughts?
-Martin.
On 16/09/14 07:57, Alexander Aring wrote:
--- snap
Ignore the below one, I will only note about this... that we don't
forget that.
This code should be a generic function for increasing headroom for
decompressing headers (IPv6, next hdr's). Still issues with
consume_skb/kfree_skb here.
+ if (stat 0) {
+ kfree_skb(skb);
+ stat = NET_RX_DROP;
+ } else {
+ consume_skb(skb);
+ }
This basically works now, but it confuse developers.
Look how stat is initzialed.
There is mixed errno and NET_RX_FOO handling here. Which is part of the
complete error handling mess. And correct freeing of skb's required a
correct error handling.
The function looks now like this:
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) {
stat = -ENOMEM;
Simple assign stat = NET_RX_DROP.
break;
}
skb_cp-dev = entry-ldev;
stat = netif_rx(skb_cp);
}
rcu_read_unlock();
if (stat 0) {
remove brackets and check on NET_RX_DROP. or vice versa.
kfree_skb(skb);
stat = NET_RX_DROP;
} else {
consume_skb(skb);
}
return stat;
Now if the list is empty we check if (stat 0) with a NET_RX_FOO stuff,
we should avoid that. I mean the current situation is because somebody
mixed this stuff and that's why we have this now.
Another developers look of some code (that's what I did) and see, aaah
returning NET_RX_FOO so we can check on it, but at this situation he
need to think a little bit more what it is the correct handline because
there is still some errno conversion.
return stat;
}
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
Hi Alex,
On the lowpan_give_skb_to_devices change.
As we are iterating over a list of lowpan_devices and could potentially copy
the skb more than once, what happens if the first device returns NET_RX_DROP
and then the second time it return NET_RX_SUCCESS? The stat variable is
overwritten so stat only ever reflects the return value of netif_rx for the
last device?
Maybe it's better to completely remove the if else at the end and always
consume the skb? For the case whereskb_copy fails then we should kfree_skb,
e.g.
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);
here we should do a:
if (stat == NET_RX_DROP)
kfree_skb(skb_cp);
or? It doesn't deliver and then we could lost the pointer.
}
rcu_read_unlock();
consume_skb(skb);
return stat;
}
what are your thoughts?
for consume_skb:
for me it's ok to make this behaviour. We never deliver the skb, always
skb_cp. So if we are before the deliver call (netif_rx) this should
never failed and we should consume the skb from which we did some copies.
btw.
I see now that's skb_copy... mhhh. But this another issue. There exist
skb_clone and skb_copy. skb_clone make a copy of struct sk_buff and data
buffer is shared. I am currently not sure if we also can use a skb_clone
here instead skb_copy, because the IPv6 doesn't manipulate the data buffer
(I think it doesn't change the data buffer - only parse) I need to think
more about this, just a performance hint. But I really also doesn't know
what sense makes multiple lowpan devices for one wpan interface. :-)
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On 16/09/14 11:17, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
Hi Alex,
On the lowpan_give_skb_to_devices change.
As we are iterating over a list of lowpan_devices and could potentially copy
the skb more than once, what happens if the first device returns NET_RX_DROP
and then the second time it return NET_RX_SUCCESS? The stat variable is
overwritten so stat only ever reflects the return value of netif_rx for the
last device?
Maybe it's better to completely remove the if else at the end and always
consume the skb? For the case whereskb_copy fails then we should kfree_skb,
e.g.
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);
here we should do a:
if (stat == NET_RX_DROP)
kfree_skb(skb_cp);
or? It doesn't deliver and then we could lost the pointer.
Doesn't netif_rx always free the buffer?
}
rcu_read_unlock();
consume_skb(skb);
return stat;
}
what are your thoughts?
for consume_skb:
for me it's ok to make this behaviour. We never deliver the skb, always
skb_cp. So if we are before the deliver call (netif_rx) this should
never failed and we should consume the skb from which we did some copies.
yep
btw.
I see now that's skb_copy... mhhh. But this another issue. There exist
skb_clone and skb_copy. skb_clone make a copy of struct sk_buff and data
buffer is shared. I am currently not sure if we also can use a skb_clone
here instead skb_copy, because the IPv6 doesn't manipulate the data buffer
(I think it doesn't change the data buffer - only parse) I need to think
more about this, just a performance hint. But I really also doesn't know
what sense makes multiple lowpan devices for one wpan interface. :-)
skb_clone could be a future patch.
Also I have been wondering why there are multiple lowpan device multiplexed
onto a wpan. Again maybe a future patch.
- Alex
--
To unsubscribe from this list: send the line unsubscribe linux-wpan in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
- Martin.
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 11:28:06AM +0100, Martin Townsend wrote:
On 16/09/14 11:17, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
Hi Alex,
On the lowpan_give_skb_to_devices change.
As we are iterating over a list of lowpan_devices and could potentially
copy the skb more than once, what happens if the first device returns
NET_RX_DROP and then the second time it return NET_RX_SUCCESS? The stat
variable is overwritten so stat only ever reflects the return value of
netif_rx for the last device?
Maybe it's better to completely remove the if else at the end and always
consume the skb? For the case whereskb_copy fails then we should
kfree_skb,
e.g.
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);
here we should do a:
if (stat == NET_RX_DROP)
kfree_skb(skb_cp);
or? It doesn't deliver and then we could lost the pointer.
Doesn't netif_rx always free the buffer?
yes, you are right. [0] Now other things makes more sense for me.
Thanks.
I mean there is another deliver function netif_receive_skb and on
comment always stand Return values (usually ignored), depends on
context what you need. But netif_rx in this context is right.
Here we should not ignore the return value, because we already are in
the packet layer (the packet layer func callback). The netif_receive_skb
function we need for putting frames from the driver (some tasklet context)
into the packet layer.
[0] is some function which is called mainly after netif_rx function,
netif_receive_skb will call this function, too.
- Alex
[0] http://lxr.free-electrons.com/source/net/core/dev.c#L3280
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
[Linux-zigbee-devel] [PATCH v4 bluetooth] Fix lowpan_rcv
This series aims to fix incorrect return values in lowpan_rcv To achieve this it also refactors the receive path to 1) free skb only from lowpan_rcv and not functions that it calls 2) move skb delivery from IPHC I have only compile tested the changes for bluetooth as I don't have any HW available so would be grateful for any testing from the Bluetooth developers. I have done some minimal testing for IEEE802.15.4 Signed-off-by: Martin Townsend martin.towns...@xsilon.com --- v1 - v2: * combined first two patches into one. * split out renaming of process_data into a new patch * re-use err in lowpan_process_data and remove err_ret local variable. * removed third patch as applies to linux-wpan v2 - v3: (Updated from Alexander Aring's Review) * re-use err hadn't been included in patch. * fixed some 'goto drop' cases in lowpan_rcv that should be 'goto drop skb' * handle error codes when returning from lowpan_frag_rcv. * Refactored lowpan_give_skb_to_devices so it consumes or free's skb and then only returns NET_RX status codes. v3 - v4: (Updated from Jukka Rissanen and Alexander Aring's Reviews) * lowpan_process_data and process_data return skb or ERR_PTR, no need for skb_inout * prefer consume_skb on non error conditions. * removed spurious setting of ret in lowpan_rcv Martin Townsend (1): 6lowpan: fix incorrect return values in lowpan_rcv include/net/6lowpan.h | 4 +-- net/6lowpan/iphc.c| 73 ++- net/bluetooth/6lowpan.c | 26 +-- net/ieee802154/6lowpan_rtnl.c | 44 +++--- 4 files changed, 75 insertions(+), 72 deletions(-) -- 1.9.1 -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
[Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
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.towns...@xsilon.com
---
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
@@ -374,10 +374,11 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16
*dgram_offset)
typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
- const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
- const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+struct sk_buff *
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+ const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+ const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+ u8 iphc0, u8 iphc1);
int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
unsigned short type, const void *_daddr,
const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..6ac7765 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,37 +171,6 @@ static int uncompress_context_based_src_addr(struct
sk_buff *skb,
return 0;
}
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
- struct net_device *dev, skb_delivery_cb deliver_skb)
-{
- struct sk_buff *new;
- int stat;
-
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
-
- new-protocol = htons(ETH_P_IPV6);
- new-pkt_type = PACKET_HOST;
- new-dev = dev;
-
- raw_dump_table(__func__, raw skb data dump before receiving,
- new-data, new-len);
-
- stat = deliver_skb(new, dev);
-
- kfree_skb(new);
-
- return stat;
-}
-
/* Uncompress function for multicast destination address,
* when M bit is set.
*/
@@ -332,14 +301,16 @@ err:
/* TTL uncompression values */
static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
- const u8 *saddr, const u8 saddr_type, const u8
saddr_len,
- const u8 *daddr, const u8 daddr_type, const u8
daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+struct sk_buff *
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+ const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+ const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+ u8 iphc0, u8 iphc1)
{
struct ipv6hdr hdr = {};
u8 tmp, num_context = 0;
int err;
+ struct sk_buff *new;
raw_dump_table(__func__, raw skb data dump uncompressed,
skb-data, skb-len);
@@ -348,7 +319,7 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
if (iphc1 LOWPAN_IPHC_CID) {
pr_debug(CID flag is set, increase header with one\n);
if (lowpan_fetch_skb(skb, num_context, sizeof(num_context)))
- goto drop;
+ return ERR_PTR(-EINVAL);
}
hdr.version = 6;
@@ -360,7 +331,7 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
*/
case 0: /* 00b */
if (lowpan_fetch_skb(skb, tmp, sizeof(tmp)))
- goto drop;
+ return ERR_PTR(-EINVAL);
memcpy(hdr.flow_lbl, skb-data[0], 3);
skb_pull(skb, 3);
@@ -373,7 +344,7 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
*/
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Just noticed a typo in the commit msg, refacored != refactored. I'll fix in v5
after further comments.
- Martin.
On 16/09/14 12:01, 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.towns...@xsilon.com
---
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
@@ -374,10 +374,11 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16
*dgram_offset)
typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
- const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
- const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+struct sk_buff *
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+ const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+ const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+ u8 iphc0, u8 iphc1);
int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
unsigned short type, const void *_daddr,
const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..6ac7765 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,37 +171,6 @@ static int uncompress_context_based_src_addr(struct
sk_buff *skb,
return 0;
}
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
-struct net_device *dev, skb_delivery_cb deliver_skb)
-{
- struct sk_buff *new;
- int stat;
-
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
-
- new-protocol = htons(ETH_P_IPV6);
- new-pkt_type = PACKET_HOST;
- new-dev = dev;
-
- raw_dump_table(__func__, raw skb data dump before receiving,
-new-data, new-len);
-
- stat = deliver_skb(new, dev);
-
- kfree_skb(new);
-
- return stat;
-}
-
/* Uncompress function for multicast destination address,
* when M bit is set.
*/
@@ -332,14 +301,16 @@ err:
/* TTL uncompression values */
static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
- const u8 *saddr, const u8 saddr_type, const u8
saddr_len,
- const u8 *daddr, const u8 daddr_type, const u8
daddr_len,
- u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+struct sk_buff *
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+ const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+ const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+ u8 iphc0, u8 iphc1)
{
struct ipv6hdr hdr = {};
u8 tmp, num_context = 0;
int err;
+ struct sk_buff *new;
raw_dump_table(__func__, raw skb data dump uncompressed,
skb-data, skb-len);
@@ -348,7 +319,7 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
if (iphc1 LOWPAN_IPHC_CID) {
pr_debug(CID flag is set, increase header with one\n);
if (lowpan_fetch_skb(skb, num_context, sizeof(num_context)))
- goto drop;
+ return ERR_PTR(-EINVAL);
}
hdr.version = 6;
@@ -360,7 +331,7 @@ int lowpan_process_data(struct sk_buff *skb, struct
net_device *dev,
*/
case 0: /* 00b */
if (lowpan_fetch_skb(skb, tmp, sizeof(tmp)))
- goto drop;
+ return ERR_PTR(-EINVAL);
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
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.towns...@xsilon.com
---
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, ...);
+
+ if (give_skb_to_upper(local_skb, dev)
also here local_skb should be skb, or?
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
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.towns...@xsilon.com
---
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, ...);
yes you are right, my mistake, I'll fix in next series.
+
+if (give_skb_to_upper(local_skb, dev)
also here local_skb should be skb, or?
- Alex
- Martin.
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
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.towns...@xsilon.com
---
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
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote: 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: ... 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? I mean sometimes we do this *skb = *new and skb is the parameter and before we did a consume_skb(skb); then local_skb is already freed after this and returning an errno and we make kfree_skb(local_skb) will crash something, I suppose. - Alex -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote: On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote: 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: ... 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? I mean sometimes we do this *skb = *new and skb is the parameter and before we did a consume_skb(skb); then local_skb is already freed after this and returning an errno and we make kfree_skb(local_skb) will crash something, I suppose. I meant skb = new for the expand skb thing. And we can't never free kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if we need a kfree_skb(local_skb) or not, because we do a consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data. - Alex -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote: On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote: On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote: 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: ... 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? I mean sometimes we do this *skb = *new and skb is the parameter and before we did a consume_skb(skb); then local_skb is already freed after this and returning an errno and we make kfree_skb(local_skb) will crash something, I suppose. I meant skb = new for the expand skb thing. And we can't never free kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if we need a kfree_skb(local_skb) or not, because we do a consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data. This all comes now in, because the ERR_PTR conversion. So we have two choices: - drop the ERR_PTR convertsion and make old behaviour - handle consume_skb/kfree_skb inside lowpan_process_data - Alex -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On 16/09/14 13:18, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
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:
...
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?
I mean sometimes we do this *skb = *new and skb is the parameter and before
we
did a consume_skb(skb); then local_skb is already freed after this and
returning an errno and we make kfree_skb(local_skb) will crash something,
I suppose.
I meant skb = new for the expand skb thing. And we can't never free
kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
we need a kfree_skb(local_skb) or not, because we do a
consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
This all comes now in, because the ERR_PTR conversion. So we have two
choices:
- drop the ERR_PTR convertsion and make old behaviour
- handle consume_skb/kfree_skb inside lowpan_process_data
- Alex
How about a label for drop_local_skb?
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
local_skb = skb_clone(skb, GFP_ATOMIC);
if (!local_skb)
goto drop;
local_skb = process_data(local_skb, dev, chan);
if (IS_ERR(local_skb))
goto drop_local_skb;
local_skb-protocol = htons(ETH_P_IPV6);
local_skb-pkt_type = PACKET_HOST;
if (give_skb_to_upper(local_skb, dev)
!= NET_RX_SUCCESS) {
kfree_skb(local_skb);
goto drop;
}
dev-stats.rx_bytes += skb-len;
dev-stats.rx_packets++;
kfree_skb(skb);
break;
default:
break;
}
}
return NET_RX_SUCCESS;
drop_local_skb:
kfree_skb(local_skb);
drop:
dev-stats.rx_dropped++;
kfree_skb(skb);
return NET_RX_DROP;
}
- Martin.
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 01:26:19PM +0100, Martin Townsend wrote:
Hi Alex,
On 16/09/14 13:18, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
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:
...
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?
I mean sometimes we do this *skb = *new and skb is the parameter and
before we
did a consume_skb(skb); then local_skb is already freed after this and
returning an errno and we make kfree_skb(local_skb) will crash something,
I suppose.
I meant skb = new for the expand skb thing. And we can't never free
kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
we need a kfree_skb(local_skb) or not, because we do a
consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
This all comes now in, because the ERR_PTR conversion. So we have two
choices:
- drop the ERR_PTR convertsion and make old behaviour
- handle consume_skb/kfree_skb inside lowpan_process_data
- Alex
How about a label for drop_local_skb?
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
local_skb = skb_clone(skb, GFP_ATOMIC);
if (!local_skb)
goto drop;
local_skb = process_data(local_skb, dev, chan);
if (IS_ERR(local_skb))
goto drop_local_skb;
local_skb-protocol = htons(ETH_P_IPV6);
local_skb-pkt_type = PACKET_HOST;
if (give_skb_to_upper(local_skb, dev)
!= NET_RX_SUCCESS) {
kfree_skb(local_skb);
goto drop;
}
dev-stats.rx_bytes += skb-len;
dev-stats.rx_packets++;
kfree_skb(skb);
break;
default:
break;
}
}
return NET_RX_SUCCESS;
drop_local_skb:
kfree_skb(local_skb);
no this can't work, when IS_ERR(local_skb) is true, local_skb is an
invalid pointer some ((void *) -errno), you can rescue it with if
(!IS_ERR(local_skb)), but... I don't know it looks complicated. :-)
What I mean is in lowpan_process_data you have a paramater skb and a skb
as return value.
Sometimes we need a consume_skb($PARAMETER_SKB), because we make the
copy_expand. After this the $PARAMETER_SKB is invalid and we have the
$RETURN_SKB as our new skb.
We don't know here if we need a kfree_skb($PARAMETER_SKB) or not because
we don't know if we did a consume_skb($PARAMETER_SKB). I think the error
handling need to be in lowpan_process_data again or make something which
handle this case.
I hope it was understandable what I mean here.
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On 16/09/14 13:34, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:26:19PM +0100, Martin Townsend wrote:
Hi Alex,
On 16/09/14 13:18, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
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:
...
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?
I mean sometimes we do this *skb = *new and skb is the parameter and
before we
did a consume_skb(skb); then local_skb is already freed after this and
returning an errno and we make kfree_skb(local_skb) will crash something,
I suppose.
I meant skb = new for the expand skb thing. And we can't never free
kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
we need a kfree_skb(local_skb) or not, because we do a
consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
This all comes now in, because the ERR_PTR conversion. So we have two
choices:
- drop the ERR_PTR convertsion and make old behaviour
- handle consume_skb/kfree_skb inside lowpan_process_data
- Alex
How about a label for drop_local_skb?
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
local_skb = skb_clone(skb, GFP_ATOMIC);
if (!local_skb)
goto drop;
local_skb = process_data(local_skb, dev, chan);
if (IS_ERR(local_skb))
goto drop_local_skb;
local_skb-protocol = htons(ETH_P_IPV6);
local_skb-pkt_type = PACKET_HOST;
if (give_skb_to_upper(local_skb, dev)
!= NET_RX_SUCCESS) {
kfree_skb(local_skb);
goto drop;
}
dev-stats.rx_bytes += skb-len;
dev-stats.rx_packets++;
kfree_skb(skb);
break;
default:
break;
}
}
return NET_RX_SUCCESS;
drop_local_skb:
kfree_skb(local_skb);
no this can't work, when IS_ERR(local_skb) is true, local_skb is an
invalid pointer some ((void *) -errno), you can rescue it with if
(!IS_ERR(local_skb)), but... I don't know it looks complicated. :-)
What I mean is in lowpan_process_data you have a paramater skb and a skb
as return value.
Sometimes we need a consume_skb($PARAMETER_SKB), because we make the
copy_expand. After this the $PARAMETER_SKB is invalid and we have the
$RETURN_SKB as our new skb.
We don't know here if we need a kfree_skb($PARAMETER_SKB) or not because
we don't know if we did a consume_skb($PARAMETER_SKB). I think the error
handling need to be in lowpan_process_data again or make something which
handle this case.
I hope it was understandable what I mean here.
- Alex
Yes I see the problem now, maybe it's better to revert back to skb_inout, less
chance of introducing bugs and then we have a well defined return value.
- Martin.
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Martin,
On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
...
Yes I see the problem now, maybe it's better to revert back to skb_inout,
less chance of introducing bugs and then we have a well defined return value.
No problem, for me it's okay, if this is okay for Jukka, we can change
it later to a better behaviour. Jukka please answer what you think about this.
I also did a small c example because this now:
char *foo(char *buf)
{
char *new;
if (some_error)
return NULL;
if (some_error)
return NULL;
new = expand(buf, 23);
if (!new)
return NULL;
free(buf);
buf = new;
/* buf is now different than the parameter buf */
if (some_error)
return NULL;
return buf;
}
int main(int argc, const char *argv[])
{
char *local_buf = malloc(42);
char *buf;
buf = foo(local_buf);
if (!buf) {
/* BUG */
/* we don't know if local_buf is still valid. */
free(local_buf);
}
return 0;
}
I think if you do buf = foo(buf) you can rescue it but this doesn't
look like a clean solution for me.
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On ti, 2014-09-16 at 14:48 +0200, Alexander Aring wrote:
Hi Martin,
On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
...
Yes I see the problem now, maybe it's better to revert back to skb_inout,
less chance of introducing bugs and then we have a well defined return
value.
No problem, for me it's okay, if this is okay for Jukka, we can change
it later to a better behaviour. Jukka please answer what you think about this.
What about doing things like this in your example?
I also did a small c example because this now:
char *foo(char *buf)
{
char *new;
if (some_error)
return NULL;
In this case you should probably not return NULL but something like
-EINVAL
if (some_error) {
free(buf);
return -EINVAL;
}
if (some_error)
return NULL;
Ditto
new = expand(buf, 23);
if (!new)
return NULL;
if (!new) {
free(buf);
return -ENOMEM;
}
free(buf);
buf = new;
/* buf is now different than the parameter buf */
if (some_error)
return NULL;
if (some_error) {
free(buf);
return -EFOOBAR;
}
return buf;
}
int main(int argc, const char *argv[])
{
char *local_buf = malloc(42);
char *buf;
buf = foo(local_buf);
if (!buf) {
/* BUG */
/* we don't know if local_buf is still valid. */
free(local_buf);
}
if (IS_ERR_OR_NULL(buf)) {
fail();
} else
free(buf);
return 0;
}
I think if you do buf = foo(buf) you can rescue it but this doesn't
look like a clean solution for me.
- Alex
In this simplified example, the subroutine frees the buf which does not
look nice I have to admit.
Cheers,
Jukka
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On ti, 2014-09-16 at 15:32 +0200, Alexander Aring wrote:
Hi Jukka,
On Tue, Sep 16, 2014 at 04:20:19PM +0300, Jukka Rissanen wrote:
Hi Alex,
On ti, 2014-09-16 at 14:48 +0200, Alexander Aring wrote:
Hi Martin,
On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
...
Yes I see the problem now, maybe it's better to revert back to
skb_inout, less chance of introducing bugs and then we have a well
defined return value.
No problem, for me it's okay, if this is okay for Jukka, we can change
it later to a better behaviour. Jukka please answer what you think about
this.
What about doing things like this in your example?
ehm yes, the example is only there to describe the current situation.
I also did a small c example because this now:
char *foo(char *buf)
{
char *new;
if (some_error)
return NULL;
In this case you should probably not return NULL but something like
-EINVAL
if (some_error) {
free(buf);
return -EINVAL;
}
yes, that's the second choice, let do consume_skb/kfree_skb inside
lowpan_process_data function.
if (some_error)
return NULL;
Ditto
new = expand(buf, 23);
if (!new)
return NULL;
if (!new) {
free(buf);
return -ENOMEM;
}
free(buf);
buf = new;
/* buf is now different than the parameter buf */
if (some_error)
return NULL;
if (some_error) {
free(buf);
return -EFOOBAR;
}
return buf;
}
int main(int argc, const char *argv[])
{
char *local_buf = malloc(42);
char *buf;
buf = foo(local_buf);
if (!buf) {
/* BUG */
/* we don't know if local_buf is still valid. */
free(local_buf);
}
if (IS_ERR_OR_NULL(buf)) {
fail();
} else
free(buf);
return 0;
}
I think if you do buf = foo(buf) you can rescue it but this doesn't
look like a clean solution for me.
- Alex
In this simplified example, the subroutine frees the buf which does not
look nice I have to admit.
I am also fine with this solution. Make something I will review it and
look if we run into trouble.
In my last mails stands, that we have two choices:
- make the skb_inout thingy
- handle error freeing into lowpan_process_data function.
You described the last one now. :-)
Great, your example clarified the issue nicely :)
I would vote for option 2) but if it makes the code too ugly then 1) is
ok too.
Cheers,
Jukka
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Jukka and Martin, On Tue, Sep 16, 2014 at 04:52:50PM +0300, Jukka Rissanen wrote: ... Great, your example clarified the issue nicely :) I would vote for option 2) but if it makes the code too ugly then 1) is ok too. I begin to have the feeling like there is a reason because there are different indicators for consume_skb, kfree_skb. Error or not error, because it's hard to implement it in some way to make a correct handling without using a pointer of pointer. A pointer of pointer always means also a unnecessary dereferencing (netdev people doesn't like unnecessary dereferencing stuff, takes too much time). That's why I vote also for option 2)... but we can also clarify this on the netdev mailinglist and ask other networking kernel hackers. - Alex -- Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk ___ Linux-zigbee-devel mailing list Linux-zigbee-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
I would like to keep freeing skb's out of process_data as process_data will
become something like iphc_decompress_hdr and it would be good if that's all it
did. Otherwise I feel we are going to put a constraint on all future header
decompression routines in that they must free the skb on error. I think it
would be better to defer this so on error you might want to try something else
with the skb, maybe not but at least the option is there.
So how about
struct sk_buff * ret_skb;
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC:/* ipv6 datagram */
ret_skb = process_data(skb, hdr);
if (IS_ERR(ret_skb))
goto drop_skb;
else
skb = ret_skb;
break;
I know we currently have 3 calls to process_data so it will look fairly ugly in
this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6
packets that are fragmented there will only be one call to process_data so it
won't look so bad. You could even wrap it in a macro but I'm not a fan of this
as they can obfuscate the code a bit.
Thoughts?
- Martin.
On 16/09/14 15:05, Alexander Aring wrote:
Hi Jukka and Martin,
On Tue, Sep 16, 2014 at 04:52:50PM +0300, Jukka Rissanen wrote:
...
Great, your example clarified the issue nicely :)
I would vote for option 2) but if it makes the code too ugly then 1) is
ok too.
I begin to have the feeling like there is a reason because there are
different indicators for consume_skb, kfree_skb. Error or not error,
because it's hard to implement it in some way to make a correct handling
without using a pointer of pointer. A pointer of pointer always means also
a unnecessary dereferencing (netdev people doesn't like unnecessary
dereferencing stuff, takes too much time).
That's why I vote also for option 2)... but we can also clarify this on
the netdev mailinglist and ask other networking kernel hackers.
- Alex
--
To unsubscribe from this list: send the line unsubscribe linux-wpan in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Martin,
On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
I would like to keep freeing skb's out of process_data as process_data will
become something like iphc_decompress_hdr and it would be good if that's all
it did. Otherwise I feel we are going to put a constraint on all future
header decompression routines in that they must free the skb on error. I
think it would be better to defer this so on error you might want to try
something else with the skb, maybe not but at least the option is there.
So how about
struct sk_buff * ret_skb;
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC:/* ipv6 datagram */
ret_skb = process_data(skb, hdr);
if (IS_ERR(ret_skb))
goto drop_skb;
else
skb = ret_skb;
break;
I know we currently have 3 calls to process_data so it will look fairly ugly
in this patch but in my next patch to fix lowpan_rcv to handle uncompressed
IPv6 packets that are fragmented there will only be one call to process_data
so it won't look so bad. You could even wrap it in a macro but I'm not a fan
of this as they can obfuscate the code a bit.
Thoughts?
sorry, I can't follow how this solve the issue if the parameter skb is
already consumed or not. If process_data returns a error before
parameter consume, then we should run kfree_skb(parameter_skb), if it's
afterwards we should do nothing. Point is we don't know that there. I
suppose if we do consume_skb and refcount reach 0 the parameter_skb
becomes a dangling pointer.
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 07:57:27PM +0100, Martin Townsend wrote:
Hi Alex,
On 16/09/14 18:38, Alexander Aring wrote:
Hi Martin,
On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
I would like to keep freeing skb's out of process_data as process_data will
become something like iphc_decompress_hdr and it would be good if that's
all it did. Otherwise I feel we are going to put a constraint on all
future header decompression routines in that they must free the skb on
error. I think it would be better to defer this so on error you might want
to try something else with the skb, maybe not but at least the option is
there.
So how about
struct sk_buff * ret_skb;
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC:/* ipv6 datagram */
ret_skb = process_data(skb, hdr);
if (IS_ERR(ret_skb))
goto drop_skb;
else
skb = ret_skb;
break;
I know we currently have 3 calls to process_data so it will look fairly
ugly in this patch but in my next patch to fix lowpan_rcv to handle
uncompressed IPv6 packets that are fragmented there will only be one call
to process_data so it won't look so bad. You could even wrap it in a macro
but I'm not a fan of this as they can obfuscate the code a bit.
Thoughts?
sorry, I can't follow how this solve the issue if the parameter skb is
already consumed or not. If process_data returns a error before
parameter consume, then we should run kfree_skb(parameter_skb), if it's
afterwards we should do nothing. Point is we don't know that there. I
suppose if we do consume_skb and refcount reach 0 the parameter_skb
becomes a dangling pointer.
- Alex
process_data never consumes the skb, it may copy_expand and then consume the
old one so it will either return an error or an skb that contains the
uncompressed ipv6 header. By calling process_data using a different sk_buff
pointer (ret_skb) that the parameter we can check this for an error and if
so goto drop_skb which will kfree_skb(skb) which is fine as skb is still
are you sure it's still valid? I don't get it. :-(
valid. if ret_skb is good and we assign to skb and carry on to the
function that passes the skb up the stack, lowpan_give_skb_to_devices,
which deals with either consuming or kfreeing.
Or am I missing something?
I make another c example, hopeful more correct than the last one:
char *foo(char *skb)
{
char *new;
if (some_error_before_consume)
return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */
/* UDP expand */
new = expand(skb, 16);
if (!new)
return ERR_PTR(-ENOMEM);
consume(skb); /* parameter skb becomes dangling pointer */
skb = new; /* doesn't rescue it, it is different than skb from caller
function
at this point, the skb_inout had rescue it, because it
was a pointer
of pointer */
/* IPv6 expand */
new = expand(skb, 40);
if (!new) /* some error after a consume(skb), will crash at drop_skb
label */
return ERR_PTR(-ENOMEM);
consume(skb);
skb = new;
return skb;
}
int main(int argc, const char *argv[])
{
char *local_buf = malloc(42);
char *skb;
local_skb = foo(skb);
if (IS_ERR(local_skb))
goto drop_skb;
else
skb = local_skb; /* ??? */
return NET_RX_SUCCESS;
drop_skb:
free(skb); /* dangling pointer will be freed if foo called consume(skb)
it's correct when foo returned on
some_error_before_consume
condition. */
drop:
return NET_RX_DROP;
}
I don't know what skb = local_skb did now there.
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
Another idea that just occured to me is to use the control buffer (cb)
in the skb to store the (de)compress_status variable. This should be
possible as it is only valid for this layer. Then process_data (or
iphc_header_decompress as I like to think of it now) will only ever
return an skb pointer; either the same one passed in (no copy_expand) or
more likely a new one (as we will skb_copy_expand to make room for
decompression). On failure process_data or functions called by this set
the decompress status variable in skb-cb for lowpan_rcv to check.
- Martin.
On 16/09/14 18:38, Alexander Aring wrote:
Hi Martin,
On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
I would like to keep freeing skb's out of process_data as process_data will
become something like iphc_decompress_hdr and it would be good if that's all
it did. Otherwise I feel we are going to put a constraint on all future
header decompression routines in that they must free the skb on error. I
think it would be better to defer this so on error you might want to try
something else with the skb, maybe not but at least the option is there.
So how about
struct sk_buff * ret_skb;
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC:/* ipv6 datagram */
ret_skb = process_data(skb, hdr);
if (IS_ERR(ret_skb))
goto drop_skb;
else
skb = ret_skb;
break;
I know we currently have 3 calls to process_data so it will look fairly ugly
in this patch but in my next patch to fix lowpan_rcv to handle uncompressed
IPv6 packets that are fragmented there will only be one call to process_data
so it won't look so bad. You could even wrap it in a macro but I'm not a
fan of this as they can obfuscate the code a bit.
Thoughts?
sorry, I can't follow how this solve the issue if the parameter skb is
already consumed or not. If process_data returns a error before
parameter consume, then we should run kfree_skb(parameter_skb), if it's
afterwards we should do nothing. Point is we don't know that there. I
suppose if we do consume_skb and refcount reach 0 the parameter_skb
becomes a dangling pointer.
- Alex
--
To unsubscribe from this list: send the line unsubscribe linux-wpan in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On 16/09/14 20:37, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 07:57:27PM +0100, Martin Townsend wrote:
Hi Alex,
On 16/09/14 18:38, Alexander Aring wrote:
Hi Martin,
On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
I would like to keep freeing skb's out of process_data as process_data
will become something like iphc_decompress_hdr and it would be good if
that's all it did. Otherwise I feel we are going to put a constraint on
all future header decompression routines in that they must free the skb on
error. I think it would be better to defer this so on error you might
want to try something else with the skb, maybe not but at least the option
is there.
So how about
struct sk_buff * ret_skb;
switch (skb-data[0] 0xe0) {
case LOWPAN_DISPATCH_IPHC:/* ipv6 datagram */
ret_skb = process_data(skb, hdr);
if (IS_ERR(ret_skb))
goto drop_skb;
else
skb = ret_skb;
break;
I know we currently have 3 calls to process_data so it will look fairly
ugly in this patch but in my next patch to fix lowpan_rcv to handle
uncompressed IPv6 packets that are fragmented there will only be one call
to process_data so it won't look so bad. You could even wrap it in a
macro but I'm not a fan of this as they can obfuscate the code a bit.
Thoughts?
sorry, I can't follow how this solve the issue if the parameter skb is
already consumed or not. If process_data returns a error before
parameter consume, then we should run kfree_skb(parameter_skb), if it's
afterwards we should do nothing. Point is we don't know that there. I
suppose if we do consume_skb and refcount reach 0 the parameter_skb
becomes a dangling pointer.
- Alex
process_data never consumes the skb, it may copy_expand and then consume the
old one so it will either return an error or an skb that contains the
uncompressed ipv6 header. By calling process_data using a different sk_buff
pointer (ret_skb) that the parameter we can check this for an error and if
so goto drop_skb which will kfree_skb(skb) which is fine as skb is still
are you sure it's still valid? I don't get it. :-(
valid. if ret_skb is good and we assign to skb and carry on to the
function that passes the skb up the stack, lowpan_give_skb_to_devices,
which deals with either consuming or kfreeing.
Or am I missing something?
I make another c example, hopeful more correct than the last one:
char *foo(char *skb)
{
char *new;
if (some_error_before_consume)
return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */
/* UDP expand */
new = expand(skb, 16);
if (!new)
return ERR_PTR(-ENOMEM);
consume(skb); /* parameter skb becomes dangling pointer */
skb = new; /* doesn't rescue it, it is different than skb from
caller function
at this point, the skb_inout had rescue it, because it
was a pointer
of pointer */
/* IPv6 expand */
new = expand(skb, 40);
if (!new) /* some error after a consume(skb), will crash at drop_skb
label */
return ERR_PTR(-ENOMEM);
consume(skb);
skb = new;
return skb;
}
I see the problem now, once the skb has been copied and then an error
occurs you have to return the error and the skb has been lost. Would
using the skb-cb to store decompress status get around this problem?
int main(int argc, const char *argv[])
{
char *local_buf = malloc(42);
char *skb;
local_skb = foo(skb);
if (IS_ERR(local_skb))
goto drop_skb;
else
skb = local_skb; /* ??? */
return NET_RX_SUCCESS;
drop_skb:
free(skb); /* dangling pointer will be freed if foo called
consume(skb)
it's correct when foo returned on
some_error_before_consume
condition. */
drop:
return NET_RX_DROP;
}
I don't know what skb = local_skb did now there.
- Alex
--
To unsubscribe from this list: send the line unsubscribe linux-wpan in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
- Martin.
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
On Tue, Sep 16, 2014 at 08:53:06PM +0100, Martin Townsend wrote:
...
I make another c example, hopeful more correct than the last one:
char *foo(char *skb)
{
char *new;
if (some_error_before_consume)
return ERR_PTR(-EINVAL); /* here we need to do a free(skb)
*/
/* UDP expand */
new = expand(skb, 16);
s/16/8 , argl, doesn't matter was only an example. :-)
if (!new)
return ERR_PTR(-ENOMEM);
consume(skb); /* parameter skb becomes dangling pointer */
skb = new; /* doesn't rescue it, it is different than skb from
caller function
at this point, the skb_inout had rescue it, because
it was a pointer
of pointer */
/* IPv6 expand */
new = expand(skb, 40);
if (!new) /* some error after a consume(skb), will crash at
drop_skb label */
return ERR_PTR(-ENOMEM);
consume(skb);
skb = new;
return skb;
}
I see the problem now, once the skb has been copied and then an error occurs
you have to return the error and the skb has been lost. Would using the
skb-cb to store decompress status get around this problem?
mhhh, complicated... on 802.15.4 6LoWPAN we use the control block
information for fragmentation information. I don't know right now if we
get trouble when we add the uncompression on the fly when FRAG1 was
received.
What exactly do you mean with decompress status?
- Alex
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
Re: [Linux-zigbee-devel] [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv
Hi Alex,
On 16/09/14 21:07, Alexander Aring wrote:
On Tue, Sep 16, 2014 at 08:53:06PM +0100, Martin Townsend wrote:
...
I make another c example, hopeful more correct than the last one:
char *foo(char *skb)
{
char *new;
if (some_error_before_consume)
return ERR_PTR(-EINVAL); /* here we need to do a free(skb)
*/
/* UDP expand */
new = expand(skb, 16);
s/16/8 , argl, doesn't matter was only an example. :-)
if (!new)
return ERR_PTR(-ENOMEM);
consume(skb); /* parameter skb becomes dangling pointer */
skb = new; /* doesn't rescue it, it is different than skb from
caller function
at this point, the skb_inout had rescue it, because
it was a pointer
of pointer */
/* IPv6 expand */
new = expand(skb, 40);
if (!new) /* some error after a consume(skb), will crash at
drop_skb label */
return ERR_PTR(-ENOMEM);
consume(skb);
skb = new;
return skb;
}
I see the problem now, once the skb has been copied and then an error occurs
you have to return the error and the skb has been lost. Would using the
skb-cb to store decompress status get around this problem?
mhhh, complicated... on 802.15.4 6LoWPAN we use the control block
information for fragmentation information. I don't know right now if we
get trouble when we add the uncompression on the fly when FRAG1 was
received.
What exactly do you mean with decompress status?
An integer that either contains an error code or 0 that process_data
would set as process_data is now IPHC decompression.
- Alex
- Martin.
--
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191iu=/4140/ostg.clktrk
___
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
