[v2] net: release dst entry in dev_hard_start_xmit()

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Tue, 12 May 2009 21:26:35 +0200
> 
>> [PATCH] net: release dst entry in dev_hard_start_xmit()
>  ...
>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> 
> Applied, thanks Eric.
> 
> Eric, please followup and double-check the pppoe paths
> that Jarek mentioned.  I never saw that fully resolved.
> 

[PATCH] ppp: unset IFF_XMIT_DST_RELEASE in ppp_setup()

Jarek pointed pppoe can call back dev_queue_xmit(), and might need
skb->dst, so its safer to unset IFF_XMIT_DST_RELEASE on ppp devices.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
---

--
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

On Tue, May 19, 2009 at 09:44:14PM +0200, Eric Dumazet wrote:
> David Miller a écrit :
> > From: Eric Dumazet <dada1@cosmosbay.com>
> > Date: Tue, 12 May 2009 21:26:35 +0200
> > 
> >> [PATCH] net: release dst entry in dev_hard_start_xmit()
> >  ...
> >> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> > 
> > Applied, thanks Eric.
> > 
> > Eric, please followup and double-check the pppoe paths
> > that Jarek mentioned.  I never saw that fully resolved.
> > 
> 
> [PATCH] ppp: unset IFF_XMIT_DST_RELEASE in ppp_setup()
> 
> Jarek pointed pppoe can call back dev_queue_xmit(), and might need
> skb->dst, so its safer to unset IFF_XMIT_DST_RELEASE on ppp devices.

Hmm... Of course, this patch looks OK to me, but actually my main
concern was more general. We avoid adding such flags for each "real"
dev, but if so IMHO it would be safer to generally add them to all
"virtual" devs - needed or not. You prefer to do this only where
necessary, but it's not always clear if it's omitted on purpose or
by chance. So, now I'm wondering about xen-netfront - needlessly I
hope ;-)

Jarek P.

> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> ---
> diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
> index 8ee9142..639d11b 100644
> --- a/drivers/net/ppp_generic.c
> +++ b/drivers/net/ppp_generic.c
> @@ -1054,6 +1054,7 @@ static void ppp_setup(struct net_device *dev)
>  	dev->type = ARPHRD_PPP;
>  	dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
>  	dev->features |= NETIF_F_NETNS_LOCAL;
> +	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
>  }
>  
>  /*
> 
--
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

Jarek Poplawski a écrit :
> On Tue, May 19, 2009 at 09:44:14PM +0200, Eric Dumazet wrote:
>> David Miller a écrit :
>>> From: Eric Dumazet <dada1@cosmosbay.com>
>>> Date: Tue, 12 May 2009 21:26:35 +0200
>>>
>>>> [PATCH] net: release dst entry in dev_hard_start_xmit()
>>>  ...
>>>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>>> Applied, thanks Eric.
>>>
>>> Eric, please followup and double-check the pppoe paths
>>> that Jarek mentioned.  I never saw that fully resolved.
>>>
>> [PATCH] ppp: unset IFF_XMIT_DST_RELEASE in ppp_setup()
>>
>> Jarek pointed pppoe can call back dev_queue_xmit(), and might need
>> skb->dst, so its safer to unset IFF_XMIT_DST_RELEASE on ppp devices.
> 
> Hmm... Of course, this patch looks OK to me, but actually my main
> concern was more general. We avoid adding such flags for each "real"
> dev, but if so IMHO it would be safer to generally add them to all
> "virtual" devs - needed or not. You prefer to do this only where
> necessary, but it's not always clear if it's omitted on purpose or
> by chance. So, now I'm wondering about xen-netfront - needlessly I
> hope ;-)
> 

This is the deal in fact, tracking all valid uses, and I'll check this.

Another path would have to set the flag only for fast devices (Gb and 10Gb)



--
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

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 19 May 2009 23:09:37 +0200

> Hmm... Of course, this patch looks OK to me, but actually my main
> concern was more general. We avoid adding such flags for each "real"
> dev, but if so IMHO it would be safer to generally add them to all
> "virtual" devs - needed or not. You prefer to do this only where
> necessary, but it's not always clear if it's omitted on purpose or
> by chance. So, now I'm wondering about xen-netfront - needlessly I
> hope ;-)

It is an issue that surely needs to be fleshed out, to make
sure we use this where possible without breaking odd cases
too easily.

But for now I'm applying Eric's patch because it does fix
something until we have these issues sorted more generally.
--
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