Messages in this thread | | | Date | Tue, 05 Jan 2010 18:16:58 -0500 | From | Michael Breuer <> | Subject | Re: [PATCH] af_packet: Don't use skb after dev_queue_xmit() |
| |
On 1/5/2010 6:07 PM, Jarek Poplawski wrote: > David Miller wrote, On 12/27/2009 05:11 AM: > > >> From: David Miller<davem@davemloft.net> >> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST) >> >> >>> From: Stephen Hemminger<shemminger@linux-foundation.org> >>> 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@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); >
| |