BUG: NIU driver: strange issues with multicast "UDP: short packet"

BUG: NIU driver: strange issues with multicast "UDP: short packet"

[Jesper Dangaard Brouer]

I have this strange issues with multicast and the NIU driver (Sun
neptune Quad NIC).  When receiving multicast traffic the kernel log
spams with something like this:

  __ratelimit: 5071 callbacks suppressed
  UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
  UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  __ratelimit: 3555 callbacks suppressed
  UDP: short packet: From 81.161.2.106:0 49320/1324 to 233.123.173.7:0
  UDP: short packet: From 81.161.2.106:0 49320/1324 to 233.123.173.7:0
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
  UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931

Tcpdumping the packets, the contents of the packets are correct.

The next strange thing is that I can make the log messages go away, if I
setup a static multicast route out another interface.

  smcroute -a eth52 81.161.2.106 233.123.173.7 eth21

Any hints whats going wrong?

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Tue, 03 Feb 2009 14:38:20 +0100

>   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
>   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
>   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
>   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
>   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
>   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931

The UDP header length field is garbage in all of these packets.

In the first two packets here, the source and dest ports are
both zero.  This is also garbage.,

> Tcpdumping the packets, the contents of the packets are correct.

So something corrupts the packet on the way to UDP input.

> The next strange thing is that I can make the log messages go away, if I
> setup a static multicast route out another interface.
> 
>   smcroute -a eth52 81.161.2.106 233.123.173.7 eth21

That could be a good clue.

Do you happen to have multicast routing enabled on this machine?  If
the multicast destination is non-local and IN_DEV_FORWARD is set on
the interface, that puts the packet through ip_mr_input.

ip_mr_input() will clone the SKB if it should be delivered locally
as well as be forwarded.

If the packet does get multicast forwarded, there is all kinds of
funny code that mangles the packet, for example ipmr_cache_report().

Any of this could be corrupting what UDP ends up seeing.

To be honest all of the SKB handling in the ipmr.c file is very scary.

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[Jesper Dangaard Brouer]

On Tue, 2009-02-03 at 15:38 -0800, David Miller wrote:
> From: Jesper Dangaard Brouer <jdb@comx.dk>
> Date: Tue, 03 Feb 2009 14:38:20 +0100
> 
> >   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
> >   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
> >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> 
> The UDP header length field is garbage in all of these packets.

Yes, but this only happens on the NIU/netpune NIC.  I works with the igb
driver, I have both a 82575 and a 82576 NIC.


> In the first two packets here, the source and dest ports are
> both zero.  This is also garbage.,
> 
> > Tcpdumping the packets, the contents of the packets are correct.
> 
> So something corrupts the packet on the way to UDP input.
> 
> > The next strange thing is that I can make the log messages go away, if I
> > setup a static multicast route out another interface.
> > 
> >   smcroute -a eth52 81.161.2.106 233.123.173.7 eth21
> 
> That could be a good clue.
> 
> Do you happen to have multicast routing enabled on this machine?

Yes
CONFIG_IP_MULTICAST=y

Looking through ipmr.c, I should also tell you that I have enabled:

CONFIG_IP_PIMSM_V1=y
CONFIG_IP_PIMSM_V2=y


> If the multicast destination is non-local and IN_DEV_FORWARD is set on
> the interface, that puts the packet through ip_mr_input.

Don't you mean IN_DEV_MFORWARD ?

> ip_mr_input() will clone the SKB if it should be delivered locally
> as well as be forwarded.

Interesting another path is choosen for mc forwarding.

If I setup a bridge for L2 forward the packets, what path is choosen
then?


> If the packet does get multicast forwarded, there is all kinds of
> funny code that mangles the packet, for example ipmr_cache_report().
> 
> Any of this could be corrupting what UDP ends up seeing.
> 
> To be honest all of the SKB handling in the ipmr.c file is very scary.

Hmmm, that doesn't sound very reassuring... 

See you around...
-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Wed, 04 Feb 2009 09:55:04 +0100

> On Tue, 2009-02-03 at 15:38 -0800, David Miller wrote:
> > From: Jesper Dangaard Brouer <jdb@comx.dk>
> > Date: Tue, 03 Feb 2009 14:38:20 +0100
> > 
> > >   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
> > >   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
> > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > 
> > The UDP header length field is garbage in all of these packets.
> 
> Yes, but this only happens on the NIU/netpune NIC.  I works with the igb
> driver, I have both a 82575 and a 82576 NIC.

Right, and what's unique about NIU is that NIU won't prepull
the protocol headers into the linear area on receive.

At the point when IPMR is dealing with potentially forwarding
the frame, the UDP headers aren't yet pulled into the linear
area.  UDP input will do that with it's pskb_may_pull() call.

I think this is the critical bit, and some part of the IPMR
code makes assumptions about all of the protocol headers being
pulled into the linear SKB area when it executes.

I really don't have time to track this down, so what I suggest
you do is add logging to UDP multicast packets at various
locations looking for the UDP header length in the paged SKB
area to go illegal.  Move your probe points around to narrow
down the culprit.

Good luck.

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[Jesper Dangaard Brouer]

On Wed, 2009-02-04 at 01:00 -0800, David Miller wrote:
> From: Jesper Dangaard Brouer <jdb@comx.dk>
> Date: Wed, 04 Feb 2009 09:55:04 +0100
> 
> > On Tue, 2009-02-03 at 15:38 -0800, David Miller wrote:
> > > From: Jesper Dangaard Brouer <jdb@comx.dk>
> > > Date: Tue, 03 Feb 2009 14:38:20 +0100
> > > 
> > > >   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
> > > >   UDP: short packet: From 81.161.2.106:0 44063/1324 to 233.123.173.7:0
> > > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > > >   UDP: short packet: From 81.161.2.106:8304 27493/1324 to 233.123.173.7:24931
> > > 
> > > The UDP header length field is garbage in all of these packets.
> > 
> > Yes, but this only happens on the NIU/netpune NIC.  I works with the igb
> > driver, I have both a 82575 and a 82576 NIC.
> 
> Right, and what's unique about NIU is that NIU won't prepull
> the protocol headers into the linear area on receive.

So thats the difference...

> At the point when IPMR is dealing with potentially forwarding
> the frame, the UDP headers aren't yet pulled into the linear
> area.  UDP input will do that with it's pskb_may_pull() call.

Can I get a little code hint, where IPMR is dealing with potentially
forwarding the frame?

> I think this is the critical bit, and some part of the IPMR
> code makes assumptions about all of the protocol headers being
> pulled into the linear SKB area when it executes.
> 
> I really don't have time to track this down, so what I suggest
> you do is add logging to UDP multicast packets at various
> locations looking for the UDP header length in the paged SKB
> area to go illegal.  Move your probe points around to narrow
> down the culprit.

Is there an easy way to test if the packet is corrupted?
Can you recomment a way to test it?


> Good luck.

Thanks I'm going to need it ;-)

Thanks for you quick answer.
-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Wed, 04 Feb 2009 10:32:06 +0100

> On Wed, 2009-02-04 at 01:00 -0800, David Miller wrote:
> > At the point when IPMR is dealing with potentially forwarding
> > the frame, the UDP headers aren't yet pulled into the linear
> > area.  UDP input will do that with it's pskb_may_pull() call.
> 
> Can I get a little code hint, where IPMR is dealing with potentially
> forwarding the frame?

ip_mr_forward().

> Is there an easy way to test if the packet is corrupted?
> Can you recomment a way to test it?

If UDP header length field is > 2000, you've got a problem.

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
>
> If the packet does get multicast forwarded, there is all kinds of
> funny code that mangles the packet, for example ipmr_cache_report().

That one looks alright since it'll either allocate a new skb
or at least do pskb_expand_head.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: BUG: NIU driver: strange issues with multicast "UDP: short packet"

[Jesper Dangaard Brouer]

On Wed, 2009-02-04 at 01:00 -0800, David Miller wrote:
> > Yes, but this only happens on the NIU/netpune NIC.  I works with the
> igb
> > driver, I have both a 82575 and a 82576 NIC.
> 
> Right, and what's unique about NIU is that NIU won't prepull
> the protocol headers into the linear area on receive.
> 
> At the point when IPMR is dealing with potentially forwarding
> the frame, the UDP headers aren't yet pulled into the linear
> area.  UDP input will do that with it's pskb_may_pull() call.

I think I have found the problem.  And its actually due to the
pskb_may_pull() call in net/ipv4/udp.c:__udp4_lib_rcv().

pskb_may_pull() may alter the SKB and a pointer to the UDP header was
stored before the pskb_may_pull() call.

I'll send a patch...

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

[PATCH] Fix UDP short packet false positive

[Jesper Dangaard Brouer]

The UDP header pointer assignment must happen after calling
pskb_may_pull().  As pskb_may_pull() can potentially alter the SKB
buffer.

This was exposted by running multicast traffic through the NIU driver,
as it won't prepull the protocol headers into the linear area on
receive.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/ipv4/udp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 1ab180b..cc3a0a0 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1231,7 +1231,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 		   int proto)
 {
 	struct sock *sk;
-	struct udphdr *uh = udp_hdr(skb);
+	struct udphdr *uh;
 	unsigned short ulen;
 	struct rtable *rt = (struct rtable*)skb->dst;
 	__be32 saddr = ip_hdr(skb)->saddr;
@@ -1244,6 +1244,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 		goto drop;		/* No space for header. */
 
+	uh   = udp_hdr(skb);
 	ulen = ntohs(uh->len);
 	if (ulen > skb->len)
 		goto short_packet;

Re: [PATCH] Fix UDP short packet false positive

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Thu, 05 Feb 2009 13:47:07 +0100

> 
> The UDP header pointer assignment must happen after calling
> pskb_may_pull().  As pskb_may_pull() can potentially alter the SKB
> buffer.
> 
> This was exposted by running multicast traffic through the NIU driver,
> as it won't prepull the protocol headers into the linear area on
> receive.
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

Excellent work!

Applied and queued up for -stable, thanks!

[RFC] [PATCH] Fix UDP short packet false positive

[Jesper Dangaard Brouer]

On Thu, 2009-02-05 at 15:06 -0800, David Miller wrote:
> From: Jesper Dangaard Brouer <jdb@comx.dk>
> Date: Thu, 05 Feb 2009 13:47:07 +0100
>
> > The UDP header pointer assignment must happen after calling
> > pskb_may_pull().  As pskb_may_pull() can potentially alter the SKB
> > buffer.
>
> Excellent work!

Thanks :-)

I'm wondering if the ip_hdr() pointer can be changed by the
pskb_may_pull(), but I assume it cannot as it should already be in the
linear area... right?

Well the patch below, shows what I mean...

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index cc3a0a0..7390af6 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1232,20 +1232,23 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 {
 	struct sock *sk;
 	struct udphdr *uh;
 	unsigned short ulen;
 	struct rtable *rt = (struct rtable*)skb->dst;
-	__be32 saddr = ip_hdr(skb)->saddr;
-	__be32 daddr = ip_hdr(skb)->daddr;
+	__be32 saddr;
+	__be32 daddr;
 	struct net *net = dev_net(skb->dev);
 
 	/*
 	 *  Validate the packet.
 	 */
 	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 		goto drop;		/* No space for header. */
 
+	saddr = ip_hdr(skb)->saddr;
+	daddr = ip_hdr(skb)->daddr;
+
 	uh   = udp_hdr(skb);
 	ulen = ntohs(uh->len);
 	if (ulen > skb->len)
 		goto short_packet;
 


-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [RFC] [PATCH] Fix UDP short packet false positive

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Fri, 06 Feb 2009 10:00:24 +0100

> On Thu, 2009-02-05 at 15:06 -0800, David Miller wrote:
> > From: Jesper Dangaard Brouer <jdb@comx.dk>
> > Date: Thu, 05 Feb 2009 13:47:07 +0100
> >
> > > The UDP header pointer assignment must happen after calling
> > > pskb_may_pull().  As pskb_may_pull() can potentially alter the SKB
> > > buffer.
> >
> > Excellent work!
> 
> Thanks :-)
> 
> I'm wondering if the ip_hdr() pointer can be changed by the
> pskb_may_pull(), but I assume it cannot as it should already be in the
> linear area... right?
> 
> Well the patch below, shows what I mean...

It has the same potential problem, but in this case you'd
only see corruption if the old skb->data buffer were reallocated
by another user and written into very quickly (or poison'd by
SLAB debugging).

Please respin this patch of your's with proper commit message
and signoffs, thanks!

BTW, ipv6 udp gets all of this right :-)

[PATCH] udp: Fix potential wrong ip_hdr(skb) pointers

[Jesper Dangaard Brouer]

On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> Please respin this patch of your's with proper commit message
> and signoffs, thanks!

Like the UDP header fix, pskb_may_pull() can potentially
alter the SKB buffer.  Thus the saddr and daddr, pointers
may point to the old skb->data buffer.

I haven't seen corruptions, as its only seen if the old
skb->data buffer were reallocated by another user and
written into very quickly (or poison'd by SLAB debugging).

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/ipv4/udp.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)


diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index cc3a0a0..c47c989 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1234,8 +1234,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	struct udphdr *uh;
 	unsigned short ulen;
 	struct rtable *rt = (struct rtable*)skb->dst;
-	__be32 saddr = ip_hdr(skb)->saddr;
-	__be32 daddr = ip_hdr(skb)->daddr;
+	__be32 saddr, daddr;
 	struct net *net = dev_net(skb->dev);
 
 	/*
@@ -1259,6 +1258,9 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	if (udp4_csum_init(skb, uh, proto))
 		goto csum_error;
 
+	saddr = ip_hdr(skb)->saddr;
+	daddr = ip_hdr(skb)->daddr;
+
 	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
 		return __udp4_lib_mcast_deliver(net, skb, uh,
 				saddr, daddr, udptable);

Re: [PATCH] udp: Fix potential wrong ip_hdr(skb) pointers

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Fri, 06 Feb 2009 10:55:58 +0100

> On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> > Please respin this patch of your's with proper commit message
> > and signoffs, thanks!
> 
> Like the UDP header fix, pskb_may_pull() can potentially
> alter the SKB buffer.  Thus the saddr and daddr, pointers
> may point to the old skb->data buffer.
> 
> I haven't seen corruptions, as its only seen if the old
> skb->data buffer were reallocated by another user and
> written into very quickly (or poison'd by SLAB debugging).
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

Applied, thanks!

Re: [PATCH] udp: Fix potential wrong ip_hdr(skb) pointers

[Eric Dumazet]

Jesper Dangaard Brouer a écrit :
> On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
>> Please respin this patch of your's with proper commit message
>> and signoffs, thanks!
> 
> Like the UDP header fix, pskb_may_pull() can potentially
> alter the SKB buffer.  Thus the saddr and daddr, pointers
> may point to the old skb->data buffer.
> 

I dont know... daddr and saddr are not pointers but integers.

Patch makes sense as a cleanup, but ChangeLog seems wrong ?

> I haven't seen corruptions, as its only seen if the old
> skb->data buffer were reallocated by another user and
> written into very quickly (or poison'd by SLAB debugging).
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/ipv4/udp.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index cc3a0a0..c47c989 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1234,8 +1234,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>  	struct udphdr *uh;
>  	unsigned short ulen;
>  	struct rtable *rt = (struct rtable*)skb->dst;
> -	__be32 saddr = ip_hdr(skb)->saddr;
> -	__be32 daddr = ip_hdr(skb)->daddr;
> +	__be32 saddr, daddr;
>  	struct net *net = dev_net(skb->dev);
>  
>  	/*
> @@ -1259,6 +1258,9 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>  	if (udp4_csum_init(skb, uh, proto))
>  		goto csum_error;
>  
> +	saddr = ip_hdr(skb)->saddr;
> +	daddr = ip_hdr(skb)->daddr;
> +
>  	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
>  		return __udp4_lib_mcast_deliver(net, skb, uh,
>  				saddr, daddr, udptable);
> 
>

Re: [PATCH] udp: Fix potential wrong ip_hdr(skb) pointers

[Jesper Dangaard Brouer]

On Fri, 2009-02-06 at 11:04 +0100, Eric Dumazet wrote:
> Jesper Dangaard Brouer a écrit :
> > On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> >> Please respin this patch of your's with proper commit message
> >> and signoffs, thanks!
> > 
> > Like the UDP header fix, pskb_may_pull() can potentially
> > alter the SKB buffer.  Thus the saddr and daddr, pointers
> > may point to the old skb->data buffer.
> > 
> 
> I dont know... daddr and saddr are not pointers but integers.

Yes, you are right... its only in the ipv6 code these are pointers
(which as DaveM mentioned handels it correctly).

> Patch makes sense as a cleanup, but ChangeLog seems wrong ?

Okay, lets view it as a cleanup... Its upto DaveM if he wants to fix the
commit message (or ask me the correct it, revert and reapply...)


> > I haven't seen corruptions, as its only seen if the old
> > skb->data buffer were reallocated by another user and
> > written into very quickly (or poison'd by SLAB debugging).
> > 
> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> > ---
> > 
> >  net/ipv4/udp.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index cc3a0a0..c47c989 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1234,8 +1234,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >  	struct udphdr *uh;
> >  	unsigned short ulen;
> >  	struct rtable *rt = (struct rtable*)skb->dst;
> > -	__be32 saddr = ip_hdr(skb)->saddr;
> > -	__be32 daddr = ip_hdr(skb)->daddr;
> > +	__be32 saddr, daddr;
> >  	struct net *net = dev_net(skb->dev);
> >  
> >  	/*
> > @@ -1259,6 +1258,9 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >  	if (udp4_csum_init(skb, uh, proto))
> >  		goto csum_error;
> >  
> > +	saddr = ip_hdr(skb)->saddr;
> > +	daddr = ip_hdr(skb)->daddr;
> > +
> >  	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
> >  		return __udp4_lib_mcast_deliver(net, skb, uh,
> >  				saddr, daddr, udptable);
> > 
> >
> 
> 
-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH] udp: Fix potential wrong ip_hdr(skb) pointers

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Fri, 06 Feb 2009 11:49:22 +0100

> On Fri, 2009-02-06 at 11:04 +0100, Eric Dumazet wrote:
> > Jesper Dangaard Brouer a écrit :
> > > On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> > >> Please respin this patch of your's with proper commit message
> > >> and signoffs, thanks!
> > > 
> > > Like the UDP header fix, pskb_may_pull() can potentially
> > > alter the SKB buffer.  Thus the saddr and daddr, pointers
> > > may point to the old skb->data buffer.
> > > 
> > 
> > I dont know... daddr and saddr are not pointers but integers.
> 
> Yes, you are right... its only in the ipv6 code these are pointers
> (which as DaveM mentioned handels it correctly).
> 
> > Patch makes sense as a cleanup, but ChangeLog seems wrong ?
> 
> Okay, lets view it as a cleanup... Its upto DaveM if he wants to fix the
> commit message (or ask me the correct it, revert and reapply...)

I already pushed the commit out, rebasing the tree is not an
option and a revert is super ugly so it's staying as-is.