Re: [PATCH] af_packet: Don't use skb after dev_queue_xmit()

[Jarek Poplawski <jarkao2@gmail.com>]

On Wed, Jan 06, 2010 at 03:33:05PM -0500, Michael Breuer wrote:
> On 1/6/2010 3:22 PM, Jarek Poplawski wrote:
> >On Wed, Jan 06, 2010 at 02:49:38PM -0500, Michael Breuer wrote:
> >>On 1/6/2010 2:22 AM, Jarek Poplawski wrote:
> >>>On Tue, Jan 05, 2010 at 09:36:28PM -0500, Michael Breuer wrote:
> >>>>On 1/5/2010 6:07 PM, Jarek Poplawski wrote:
> >>>>>----------------->
> >>>>>
> >>>>>Changing an skb after dev_queue_xmit() is illegal. And since it's
> >>>>>inconsistent to treat specially net_xmit_errno() non-zero return,
> >>>>>while ignoring other dev_queue_xmit() errors, there is no reason
> >>>>>to break the loop in tpacket_snd() in this case.
> >>>>>
> >>>>>With debugging by: Stephen Hemminger<shemminger@linux-foundation.org>
> >>>>>
> >>>>>Reported-by: Michael Breuer<mbreuer@majjas.com>
> >>>>>Signed-off-by: Jarek Poplawski<jarkao2@gmail.com>
> >>>>>---
> >>>>>
> >>>>>   net/packet/af_packet.c |    8 +++-----
> >>>>>   1 files changed, 3 insertions(+), 5 deletions(-)
> >>>>>
> >>>>>diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> >>>>>index e0516a2..984a1fa 100644
> >>>>>--- a/net/packet/af_packet.c
> >>>>>+++ b/net/packet/af_packet.c
> >>>>>@@ -1021,8 +1021,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >>>>>
> >>>>>   		status = TP_STATUS_SEND_REQUEST;
> >>>>>   		err = dev_queue_xmit(skb);
> >>>>>-		if (unlikely(err>    0&&    (err = net_xmit_errno(err)) != 0))
> >>>>>-			goto out_xmit;
> >>>>>+		if (unlikely(err>    0))
> >>>>>+			err = net_xmit_errno(err);
> >>>>>+
> >>>>>   		packet_increment_head(&po->tx_ring);
> >>>>>   		len_sum += tp_len;
> >>>>>   	} while (likely((ph != NULL) ||
> >>>>>@@ -1033,9 +1034,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >>>>>   	err = len_sum;
> >>>>>   	goto out_put;
> >>>>>
> >>>>>-out_xmit:
> >>>>>-	skb->destructor = sock_wfree;
> >>>>>-	atomic_dec(&po->tx_ring.pending);
> >>>>>   out_status:
> >>>>>   	__packet_set_status(po, ph, status);
> >>>>>   	kfree_skb(skb);
> >>>>>--
...
> >>This patch at first behaved similarly to the previous one - seemed
> >>to be running a bit better... until the adapter went down :(
> >I'm not sure: do you mean this patch above vs previous one by Stephen,
> >or did you manage to try my "alernative #2" patch already?
> >
> >BTW, I forgot to mention, and maybe it doesn't matter here, but it
> >would be better to (always) use my sky2 patch from Berck Nash's
> >thread.
> >
> >Jarek P.
> This was using "alternative #2" patch. I didn't get the hang with
> alternative #1. Your sky2 patch from Berck Nash's thread was
> included in both cases; Stephen's was not.

OK, so I guess "alternative #1" (above) seems safer to recommend for
now (as I assumed earlier).

On the other hand, we really don't know if it's only because it's
because it's nicer for your hardware (or still some other bug around),
so as before: let David choose ;-)

BTW, I think you could still use Stephen's patch too (there might be
still something more like this). There was also mentioned this network
manager again. I might be wrong, but IMHO there could be some
interaction even if it doesn't use this device; so could/did you try
to disable it entirely?

Thanks for testing!
Jarek P.