Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653271 > unrolled thread

[PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
    Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if  reallocated Andrew Lunn <andrew@lunn.ch> - 2017-05-30 17:30 +0200
      Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 17:50 +0200
        Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if  reallocated Andrew Lunn <andrew@lunn.ch> - 2017-05-30 18:30 +0200

#1653271 — [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

If the receive function of a tagger reallocated the SKB, the original
SKB is currently not freed. Fix this and free it on both copy or error.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 98173a3f6fd1..0b6f2c7d7d1c 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -209,11 +209,12 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
+	/* Receive function may have to reallocate the original SKB */
 	nskb = dst->tag_ops->rcv(skb, dev);
-	if (!nskb) {
+	if (nskb != skb)
 		kfree_skb(skb);
+	if (!nskb)
 		return 0;
-	}
 
 	skb = nskb;
 	skb_push(skb, ETH_HLEN);
-- 
2.13.0

[toc] | [next] | [standalone]

#1653319 — Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

On Tue, May 30, 2017 at 10:21:28AM -0400, Vivien Didelot wrote:
> If the receive function of a tagger reallocated the SKB, the original
> SKB is currently not freed. Fix this and free it on both copy or error.

I don't see any of the receive functions reallocate the skb. It might
be better to just simplify the code to take away the option to return
a different skb.

  Andrew

[toc] | [prev] | [next] | [standalone]

#1653326

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, May 30, 2017 at 10:21:28AM -0400, Vivien Didelot wrote:
>> If the receive function of a tagger reallocated the SKB, the original
>> SKB is currently not freed. Fix this and free it on both copy or error.
>
> I don't see any of the receive functions reallocate the skb. It might
> be better to just simplify the code to take away the option to return
> a different skb.

I think it was written such way to be symmetrical with the xmit
implementation, where the trailer tagger does reallocate the skb.

I would say that keeping the symmetry is simpler from the point of view
of the tagging implementations, but I don't have strong opinion here.

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1653353 — Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

On Tue, May 30, 2017 at 11:41:51AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Tue, May 30, 2017 at 10:21:28AM -0400, Vivien Didelot wrote:
> >> If the receive function of a tagger reallocated the SKB, the original
> >> SKB is currently not freed. Fix this and free it on both copy or error.
> >
> > I don't see any of the receive functions reallocate the skb. It might
> > be better to just simplify the code to take away the option to return
> > a different skb.
> 
> I think it was written such way to be symmetrical with the xmit
> implementation, where the trailer tagger does reallocate the skb.

trailer_xmit() releases the original and so does the in-review KSZ
tagger.

So i think in general, Rx and Rx, the tagger should be responsible for
freeing the original, but on error the core should do the free.

	Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web