[RFC] udp: don't rereference dst_entry dev pointer on rcv

Submitted by Eric Dumazet on March 14, 2013, 3:29 p.m.

Details

Message ID 1363274946.29475.24.camel@edumazet-glaptop
State RFC
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet March 14, 2013, 3:29 p.m.
On Thu, 2013-03-14 at 16:05 +0100, Eric Dumazet wrote:
> On Thu, 2013-03-14 at 14:45 +0000, Tom Parkin wrote:
> > On Thu, Mar 14, 2013 at 02:18:04AM +0100, Eric Dumazet wrote:
> > > Ah thanks for this, as this definitely makes more sense ;)
> > > 
> > > Could you try the following fix ?
> > > 
> > > diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> > > index b6d30ac..87f4ecb 100644
> > > --- a/net/ipv4/ip_fragment.c
> > > +++ b/net/ipv4/ip_fragment.c
> > > @@ -529,6 +529,7 @@ found:
> > >  	    qp->q.meat == qp->q.len)
> > >  		return ip_frag_reasm(qp, prev, dev);
> > >  
> > > +	skb_dst_force(skb);
> > >  	inet_frag_lru_move(&qp->q);
> > >  	return -EINPROGRESS;
> > >
> > 
> > Thanks Eric, with this patch I can no longer reproduce the oops :-)
> 
> Thanks for testing.
> 
> I am considering an alternative patch :
> 
> We can drop the reference instead, and use the dst of the last skb.
> 
> This would help to not dirty the dst refcount.
> 
> I'll send an updated version.
> 

OK, I tested the following one instead, please test it so that I can send an official patch.

Thanks



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tom Parkin March 14, 2013, 4:14 p.m.
On Thu, Mar 14, 2013 at 04:29:06PM +0100, Eric Dumazet wrote:
> OK, I tested the following one instead, please test it so that I can send an official patch.
> 
> Thanks
> 
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index b6d30ac..2d23830 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -529,6 +529,7 @@ found:
>  	    qp->q.meat == qp->q.len)
>  		return ip_frag_reasm(qp, prev, dev);
>  
> +	skb_dst_drop(skb);
>  	inet_frag_lru_move(&qp->q);
>  	return -EINPROGRESS;
>

Thanks again, Eric.

Sadly, with this patch the oops is back :-(  It falls over quite
quickly for me.
Damien Wyart March 23, 2013, 10:31 a.m.
Hi Eric,

* Tom Parkin <tparkin@katalix.com> [2013-03-14 16:14]:
> On Thu, Mar 14, 2013 at 04:29:06PM +0100, Eric Dumazet wrote:
> > OK, I tested the following one instead, please test it so that I can send an official patch.

> > Thanks

> > diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> > index b6d30ac..2d23830 100644
> > --- a/net/ipv4/ip_fragment.c
> > +++ b/net/ipv4/ip_fragment.c
> > @@ -529,6 +529,7 @@ found:
> >  	    qp->q.meat == qp->q.len)
> >  		return ip_frag_reasm(qp, prev, dev);

> > +	skb_dst_drop(skb);
> >  	inet_frag_lru_move(&qp->q);
> >  	return -EINPROGRESS;

> Thanks again, Eric.

> Sadly, with this patch the oops is back :-(  It falls over quite
> quickly for me.

Just a quick check: do you plan to have another look at this in the
coming days/weeks? You said you had a huge backlog, but in the meantime,
you sent many patches on many topics, so just wanted to check if this
one had not be forgotten for ever.

Thanks,
Eric Dumazet March 23, 2013, 3:09 p.m.
On Sat, 2013-03-23 at 11:31 +0100, Damien Wyart wrote:
> Hi Eric,
> 
> * Tom Parkin <tparkin@katalix.com> [2013-03-14 16:14]:
> > On Thu, Mar 14, 2013 at 04:29:06PM +0100, Eric Dumazet wrote:
> > > OK, I tested the following one instead, please test it so that I can send an official patch.
> 
> > > Thanks
> 
> > > diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> > > index b6d30ac..2d23830 100644
> > > --- a/net/ipv4/ip_fragment.c
> > > +++ b/net/ipv4/ip_fragment.c
> > > @@ -529,6 +529,7 @@ found:
> > >  	    qp->q.meat == qp->q.len)
> > >  		return ip_frag_reasm(qp, prev, dev);
> 
> > > +	skb_dst_drop(skb);
> > >  	inet_frag_lru_move(&qp->q);
> > >  	return -EINPROGRESS;
> 
> > Thanks again, Eric.
> 
> > Sadly, with this patch the oops is back :-(  It falls over quite
> > quickly for me.
> 
> Just a quick check: do you plan to have another look at this in the
> coming days/weeks? You said you had a huge backlog, but in the meantime,
> you sent many patches on many topics, so just wanted to check if this
> one had not be forgotten for ever.

Hi Damien. This one is part of my backlog, I will send a patch soon.

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet April 11, 2013, 4:32 p.m.
On Thu, 2013-03-14 at 16:29 +0100, Eric Dumazet wrote:
> On Thu, 2013-03-14 at 16:05 +0100, Eric Dumazet wrote:
> > On Thu, 2013-03-14 at 14:45 +0000, Tom Parkin wrote:
> > > On Thu, Mar 14, 2013 at 02:18:04AM +0100, Eric Dumazet wrote:
> > > > Ah thanks for this, as this definitely makes more sense ;)
> > > > 
> > > > Could you try the following fix ?
> > > > 
> > > > diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> > > > index b6d30ac..87f4ecb 100644
> > > > --- a/net/ipv4/ip_fragment.c
> > > > +++ b/net/ipv4/ip_fragment.c
> > > > @@ -529,6 +529,7 @@ found:
> > > >  	    qp->q.meat == qp->q.len)
> > > >  		return ip_frag_reasm(qp, prev, dev);
> > > >  
> > > > +	skb_dst_force(skb);
> > > >  	inet_frag_lru_move(&qp->q);
> > > >  	return -EINPROGRESS;
> > > >
> > > 
> > > Thanks Eric, with this patch I can no longer reproduce the oops :-)
> > 
> > Thanks for testing.
> > 
> > I am considering an alternative patch :
> > 
> > We can drop the reference instead, and use the dst of the last skb.
> > 
> > This would help to not dirty the dst refcount.
> > 
> > I'll send an updated version.
> > 
> 
> OK, I tested the following one instead, please test it so that I can send an official patch.
> 
> Thanks
> 
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index b6d30ac..2d23830 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -529,6 +529,7 @@ found:
>  	    qp->q.meat == qp->q.len)
>  		return ip_frag_reasm(qp, prev, dev);
>  
> +	skb_dst_drop(skb);
>  	inet_frag_lru_move(&qp->q);
>  	return -EINPROGRESS;
>  
> 

Short update : I do not understand yet why this patch is not working.

Normally, the reassembled packet should get the dst from the last skb
(the one completing the packet)...

I have to make more experiments.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index b6d30ac..2d23830 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -529,6 +529,7 @@  found:
 	    qp->q.meat == qp->q.len)
 		return ip_frag_reasm(qp, prev, dev);
 
+	skb_dst_drop(skb);
 	inet_frag_lru_move(&qp->q);
 	return -EINPROGRESS;