* [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment
@ 2017-02-13 19:44 Linus Lüssing
2017-02-13 20:00 ` Linus Lüssing
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Linus Lüssing @ 2017-02-13 19:44 UTC (permalink / raw)
To: b.a.t.m.a.n
Trying to split and transmit a unicast packet in 16 parts will fail for
the final fragment: After having sent the 15th one with a frag_packet.no
index of 14, we will increase the the index to 15 - and return with an
error code immediately, even though one more fragment is due for
transmission and allowed.
Fixing this issue by moving the check before incrementing the index.
While at it, adding an unlikely(), because the check is actually more of
an assertion.
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
Compile time tested only
---
net/batman-adv/fragmentation.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c
index 0854ebd..f181868 100644
--- a/net/batman-adv/fragmentation.c
+++ b/net/batman-adv/fragmentation.c
@@ -499,6 +499,12 @@ int batadv_frag_send_packet(struct sk_buff *skb,
/* Eat and send fragments from the tail of skb */
while (skb->len > max_fragment_size) {
+ /* The initial check in this function should cover this case */
+ if (unlikely(frag_header.no == BATADV_FRAG_MAX_FRAGMENTS - 1)) {
+ ret = -EINVAL;
+ goto put_primary_if;
+ }
+
skb_fragment = batadv_frag_create(skb, &frag_header, mtu);
if (!skb_fragment) {
ret = -ENOMEM;
@@ -515,12 +521,6 @@ int batadv_frag_send_packet(struct sk_buff *skb,
}
frag_header.no++;
-
- /* The initial check in this function should cover this case */
- if (frag_header.no == BATADV_FRAG_MAX_FRAGMENTS - 1) {
- ret = -EINVAL;
- goto put_primary_if;
- }
}
/* Make room for the fragment header. */
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment
2017-02-13 19:44 [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment Linus Lüssing
@ 2017-02-13 20:00 ` Linus Lüssing
2017-02-13 21:23 ` Sven Eckelmann
2017-02-13 20:51 ` Sven Eckelmann
2017-02-21 17:27 ` Sven Eckelmann
2 siblings, 1 reply; 6+ messages in thread
From: Linus Lüssing @ 2017-02-13 20:00 UTC (permalink / raw)
To: b.a.t.m.a.n
On Mon, Feb 13, 2017 at 08:44:31PM +0100, Linus Lüssing wrote:
> Trying to split and transmit a unicast packet in 16 parts will fail for
> the final fragment: After having sent the 15th one with a frag_packet.no
> index of 14, we will increase the the index to 15 - and return with an
> error code immediately, even though one more fragment is due for
> transmission and allowed.
>
> Fixing this issue by moving the check before incrementing the index.
>
> While at it, adding an unlikely(), because the check is actually more of
> an assertion.
>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
>
> ---
>
> Compile time tested only
> ---
> net/batman-adv/fragmentation.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c
> index 0854ebd..f181868 100644
> --- a/net/batman-adv/fragmentation.c
> +++ b/net/batman-adv/fragmentation.c
> @@ -499,6 +499,12 @@ int batadv_frag_send_packet(struct sk_buff *skb,
And one more thing which seems fishy to me in this function:
526 /* Make room for the fragment header. */
527 if (batadv_skb_head_push(skb, header_size) < 0 ||
528 pskb_expand_head(skb, header_size + ETH_HLEN, 0, GFP_ATOMIC) < 0) {
529 ret = -ENOMEM;
530 goto put_primary_if;
531 }
532
533 memcpy(skb->data, &frag_header, header_size);
For the pskb_expand_head() case, there is an skb_push(header_size) missing,
isn't it?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment
2017-02-13 20:00 ` Linus Lüssing
@ 2017-02-13 21:23 ` Sven Eckelmann
2017-02-14 8:30 ` Linus Lüssing
0 siblings, 1 reply; 6+ messages in thread
From: Sven Eckelmann @ 2017-02-13 21:23 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: martin
[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]
On Montag, 13. Februar 2017 21:00:08 CET Linus Lüssing wrote:
[...]
> And one more thing which seems fishy to me in this function:
>
> 526 /* Make room for the fragment header. */
> 527 if (batadv_skb_head_push(skb, header_size) < 0 ||
> 528 pskb_expand_head(skb, header_size + ETH_HLEN, 0, GFP_ATOMIC) < 0) {
> 529 ret = -ENOMEM;
> 530 goto put_primary_if;
> 531 }
> 532
> 533 memcpy(skb->data, &frag_header, header_size);
>
>
> For the pskb_expand_head() case, there is an skb_push(header_size) missing,
> isn't it?
I am a little bit confused about your remark... and about the code.
So let's check what Martin wrote:
* get header_size more room in our data section
* allocate new buffer to get header_size + ETH_HLEN in front (but not part)
of our data section
If one of these two fails then it will get in panic mode and leave the
function.
I agree that the header_size in pskb_expand_head is slightly odd and I don't
see why we would need it. My best guess would be to compensate the extra
header which "stole" some bytes from the headroom which the underlying
interface may need.
But more importantly, I don't understand why an extra skb_push(header_size)
(like you've suggested) would be necessary here. Why would you want to have an
empty header_size region in the fragment between the actual header and the
fragment data?
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment
2017-02-13 21:23 ` Sven Eckelmann
@ 2017-02-14 8:30 ` Linus Lüssing
0 siblings, 0 replies; 6+ messages in thread
From: Linus Lüssing @ 2017-02-14 8:30 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking; +Cc: martin
On Mon, Feb 13, 2017 at 10:23:52PM +0100, Sven Eckelmann wrote:
> On Montag, 13. Februar 2017 21:00:08 CET Linus Lüssing wrote:
> [...]
> > And one more thing which seems fishy to me in this function:
> >
> > 526 /* Make room for the fragment header. */
> > 527 if (batadv_skb_head_push(skb, header_size) < 0 ||
> > 528 pskb_expand_head(skb, header_size + ETH_HLEN, 0, GFP_ATOMIC) < 0) {
> > 529 ret = -ENOMEM;
> > 530 goto put_primary_if;
> > 531 }
> > 532
> > 533 memcpy(skb->data, &frag_header, header_size);
> >
> >
> > For the pskb_expand_head() case, there is an skb_push(header_size) missing,
> > isn't it?
>
> I am a little bit confused about your remark... and about the code.
>
> So let's check what Martin wrote:
>
> * get header_size more room in our data section
> * allocate new buffer to get header_size + ETH_HLEN in front (but not part)
> of our data section
>
> If one of these two fails then it will get in panic mode and leave the
> function.
Aiy, I'm sorry, misread that, you are right. Forget my remark.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment
2017-02-13 19:44 [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment Linus Lüssing
2017-02-13 20:00 ` Linus Lüssing
@ 2017-02-13 20:51 ` Sven Eckelmann
2017-02-21 17:27 ` Sven Eckelmann
2 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2017-02-13 20:51 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
On Montag, 13. Februar 2017 20:44:31 CET Linus Lüssing wrote:
> Trying to split and transmit a unicast packet in 16 parts will fail for
> the final fragment: After having sent the 15th one with a frag_packet.no
> index of 14, we will increase the the index to 15 - and return with an
> error code immediately, even though one more fragment is due for
> transmission and allowed.
>
> Fixing this issue by moving the check before incrementing the index.
>
> While at it, adding an unlikely(), because the check is actually more of
> an assertion.
>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
>
> ---
>
> Compile time tested only
Seems to make sense. But have to talk with Simon how he wants to proceed
with the maint branch regarding the net.git submissions. And we should
add The fixes line before committing.
Fixes: db56e4ecf5c2 ("batman-adv: Fragment and send skbs larger than mtu")
Reviewed-by: Sven Eckelmann <sven@narfation.org>
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment
2017-02-13 19:44 [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment Linus Lüssing
2017-02-13 20:00 ` Linus Lüssing
2017-02-13 20:51 ` Sven Eckelmann
@ 2017-02-21 17:27 ` Sven Eckelmann
2 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2017-02-21 17:27 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 792 bytes --]
On Montag, 13. Februar 2017 20:44:31 CET Linus Lüssing wrote:
> Trying to split and transmit a unicast packet in 16 parts will fail for
> the final fragment: After having sent the 15th one with a frag_packet.no
> index of 14, we will increase the the index to 15 - and return with an
> error code immediately, even though one more fragment is due for
> transmission and allowed.
>
> Fixing this issue by moving the check before incrementing the index.
>
> While at it, adding an unlikely(), because the check is actually more of
> an assertion.
>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
Applied in 464eff3b1768ff190466a453a57ac140ea5cb756 [1]
Thanks,
Sven
[1] https://git.open-mesh.org/batman-adv.git/commit/464eff3b1768ff190466a453a57ac140ea5cb756
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-21 17:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-13 19:44 [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment Linus Lüssing
2017-02-13 20:00 ` Linus Lüssing
2017-02-13 21:23 ` Sven Eckelmann
2017-02-14 8:30 ` Linus Lüssing
2017-02-13 20:51 ` Sven Eckelmann
2017-02-21 17:27 ` Sven Eckelmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox