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

[Michael Breuer]

On 1/5/2010 6:07 PM, Jarek Poplawski wrote:
> David Miller wrote, On 12/27/2009 05:11 AM:
>
>    
> > From: David Miller<davem@xxxxxxxxxxxxx>
> > Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST)
> >
> >      
> > > From: Stephen Hemminger<shemminger@xxxxxxxxxxxxxxxxxxxx>
> > > Date: Sat, 26 Dec 2009 14:05:44 -0800
> > >
> > >        
> > > > Other drivers may have same problem, I really think this ought
> > > > to be done at higher level.
> > > >          
> > > I tend to agree with you, and I thought we had handled all
> > > cases.  Let's simply make AF_PACKET linearize the link
> > > level header before sending things out to the transmit path.
> > >
> > > I can work on this if you want.
> > >        
> > Actually Stephen, I took a look and I can't see how AF_PACKET
> > can create this situation.
> >
> > It always copies into the linear area of the SKB it allocates
> > for sendmsg() processing.  Whether the data comes from sendmsg
> > data or the mmap() ring buffer.
> >      
> Actually, I think there is a bug in this place, but of course this
> might be unconnected. Anyway, Michael, could you try this patch?
> BTW, did you try with CONFIG_PACKET_MMAP disabled?
>    
I did not try with CONFIG_PACKET_MMAP disabled.

Please let me know which permutations of the following would be most 
valuable to test:
Davids patch in/out
The attached patch in/out
CONFIG_PACKET_MMAP on/off

I'm thinking your patch in, david's out, with and without 
CONFIG_PACKET_MMAP.
> Thanks,
> Jarek P.
> ----------------->
>
> 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@xxxxxxxxxxxxxxxxxxxx>
>
> Reported-by: Michael Breuer<mbreuer@xxxxxxxxxx>
> Signed-off-by: Jarek Poplawski<jarkao2@xxxxxxxxx>
> ---
>
>   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);
>    

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/