* net/sched/act_nat.c BUG
@ 2010-07-09 14:37 Rodrigo Partearroyo González
2010-07-09 15:13 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Rodrigo Partearroyo González @ 2010-07-09 14:37 UTC (permalink / raw
To: Herbert Xu, Linux Kernel Mailing List
Cc: Iratxo Pichel Ortiz, Noelia Morón
Hi all,
I have been testing Stateless NAT and found that ICMP packets with length less
than 20 bytes were not correctly NAT'ed. I have found a BUG that makes taking
into account IP header length twice, so ICMP packets smaller than 20 bytes
were being dropped.
The proposed fix is:
Index: net/sched/act_nat.c
===================================================================
--- net/sched/act_nat.c
+++ net/sched/act_nat.c
@@ -202,7 +202,7 @@
{
struct icmphdr *icmph;
- if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
+ if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
goto drop;
icmph = (void *)(skb_network_header(skb) + ihl);
Please, consider applying it.
--
Rodrigo Partearroyo González
R&D Engineer
Albentia Systems S.A.
http://www.albentia.com
+34 914400213
C\Margarita Salas 22
Parque Tecnológico de Leganés
Leganés (28918)
Madrid
Spain
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net/sched/act_nat.c BUG
2010-07-09 14:37 net/sched/act_nat.c BUG Rodrigo Partearroyo González
@ 2010-07-09 15:13 ` Eric Dumazet
2010-07-09 23:23 ` Herbert Xu
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2010-07-09 15:13 UTC (permalink / raw
To: Rodrigo Partearroyo González
Cc: Herbert Xu, Linux Kernel Mailing List, Iratxo Pichel Ortiz,
Noelia Morón, netdev
Le vendredi 09 juillet 2010 à 16:37 +0200, Rodrigo Partearroyo González
a écrit :
> Hi all,
>
> I have been testing Stateless NAT and found that ICMP packets with length less
> than 20 bytes were not correctly NAT'ed. I have found a BUG that makes taking
> into account IP header length twice, so ICMP packets smaller than 20 bytes
> were being dropped.
>
CC netdev
> The proposed fix is:
>
> Index: net/sched/act_nat.c
> ===================================================================
> --- net/sched/act_nat.c
> +++ net/sched/act_nat.c
> @@ -202,7 +202,7 @@
> {
> struct icmphdr *icmph;
>
> - if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
> + if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
> goto drop;
>
> icmph = (void *)(skb_network_header(skb) + ihl);
>
> Please, consider applying it.
Nice catch, but take a look at next lines too,
when call to skb_clone_writable() is done, since same error is present.
skb_clone_writable(skb,
ihl + sizeof(*icmph) + sizeof(*iph))
Please submit a formal patch, with your "Signed-off-by: ...", as
documented in Documentation/SubmittingPatches
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net/sched/act_nat.c BUG
2010-07-09 15:13 ` Eric Dumazet
@ 2010-07-09 23:23 ` Herbert Xu
2010-07-10 0:00 ` David Miller
2010-07-10 6:27 ` Eric Dumazet
0 siblings, 2 replies; 5+ messages in thread
From: Herbert Xu @ 2010-07-09 23:23 UTC (permalink / raw
To: Eric Dumazet
Cc: Rodrigo Partearroyo González, Linux Kernel Mailing List,
Iratxo Pichel Ortiz, Noelia Morón, netdev
On Fri, Jul 09, 2010 at 05:13:40PM +0200, Eric Dumazet wrote:
> Le vendredi 09 juillet 2010 à 16:37 +0200, Rodrigo Partearroyo González
> a écrit :
> > Hi all,
> >
> > I have been testing Stateless NAT and found that ICMP packets with length less
> > than 20 bytes were not correctly NAT'ed. I have found a BUG that makes taking
> > into account IP header length twice, so ICMP packets smaller than 20 bytes
> > were being dropped.
> >
>
> CC netdev
>
> > The proposed fix is:
> >
> > Index: net/sched/act_nat.c
> > ===================================================================
> > --- net/sched/act_nat.c
> > +++ net/sched/act_nat.c
> > @@ -202,7 +202,7 @@
> > {
> > struct icmphdr *icmph;
> >
> > - if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
> > + if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
> > goto drop;
> >
> > icmph = (void *)(skb_network_header(skb) + ihl);
> >
> > Please, consider applying it.
>
> Nice catch, but take a look at next lines too,
> when call to skb_clone_writable() is done, since same error is present.
>
> skb_clone_writable(skb,
> ihl + sizeof(*icmph) + sizeof(*iph))
>
> Please submit a formal patch, with your "Signed-off-by: ...", as
> documented in Documentation/SubmittingPatches
No we do need the second IP header, think about it...
However, we should only drop it only if it's long enough and
pskb_may_pull fails.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net/sched/act_nat.c BUG
2010-07-09 23:23 ` Herbert Xu
@ 2010-07-10 0:00 ` David Miller
2010-07-10 6:27 ` Eric Dumazet
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2010-07-10 0:00 UTC (permalink / raw
To: herbert; +Cc: eric.dumazet, rpartearroyo, linux-kernel, ipichel, nmoron, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 10 Jul 2010 07:23:59 +0800
> On Fri, Jul 09, 2010 at 05:13:40PM +0200, Eric Dumazet wrote:
>> Le vendredi 09 juillet 2010 à 16:37 +0200, Rodrigo Partearroyo González
>> a écrit :
>> > Hi all,
>> >
>> > I have been testing Stateless NAT and found that ICMP packets with length less
>> > than 20 bytes were not correctly NAT'ed. I have found a BUG that makes taking
>> > into account IP header length twice, so ICMP packets smaller than 20 bytes
>> > were being dropped.
>> >
>>
>> CC netdev
>>
>> > The proposed fix is:
>> >
>> > Index: net/sched/act_nat.c
>> > ===================================================================
>> > --- net/sched/act_nat.c
>> > +++ net/sched/act_nat.c
>> > @@ -202,7 +202,7 @@
>> > {
>> > struct icmphdr *icmph;
>> >
>> > - if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
>> > + if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
>> > goto drop;
>> >
>> > icmph = (void *)(skb_network_header(skb) + ihl);
>> >
>> > Please, consider applying it.
>>
>> Nice catch, but take a look at next lines too,
>> when call to skb_clone_writable() is done, since same error is present.
>>
>> skb_clone_writable(skb,
>> ihl + sizeof(*icmph) + sizeof(*iph))
>>
>> Please submit a formal patch, with your "Signed-off-by: ...", as
>> documented in Documentation/SubmittingPatches
>
> No we do need the second IP header, think about it...
>
> However, we should only drop it only if it's long enough and
> pskb_may_pull fails.
Ok, I've reverted until we come up with the proper fix, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net/sched/act_nat.c BUG
2010-07-09 23:23 ` Herbert Xu
2010-07-10 0:00 ` David Miller
@ 2010-07-10 6:27 ` Eric Dumazet
1 sibling, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2010-07-10 6:27 UTC (permalink / raw
To: Herbert Xu
Cc: Rodrigo Partearroyo González, Linux Kernel Mailing List,
Iratxo Pichel Ortiz, Noelia Morón, netdev
Le samedi 10 juillet 2010 à 07:23 +0800, Herbert Xu a écrit :
> No we do need the second IP header, think about it...
>
> However, we should only drop it only if it's long enough and
> pskb_may_pull fails.
Indeed right you are !
As Changli suggested, we need another pskb_may_pull() call.
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-07-10 6:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-09 14:37 net/sched/act_nat.c BUG Rodrigo Partearroyo González
2010-07-09 15:13 ` Eric Dumazet
2010-07-09 23:23 ` Herbert Xu
2010-07-10 0:00 ` David Miller
2010-07-10 6:27 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).