af_packet: Don't use skb after dev_queue_xmit()
in [Kernel]
|
Prev: New HID device Philips Remote RC 153_Vista
Next: tpm_infineon: Fix suspend/resume handler for pnp_driver
From: Michael Breuer on 5 Jan 2010 18:20 On 1/5/2010 6:07 PM, Jarek Poplawski wrote: > David Miller wrote, On 12/27/2009 05:11 AM: > > >> From: David Miller<davem(a)davemloft.net> >> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST) >> >> >>> From: Stephen Hemminger<shemminger(a)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(a)linux-foundation.org> > > Reported-by: Michael Breuer<mbreuer(a)majjas.com> > Signed-off-by: Jarek Poplawski<jarkao2(a)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); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jarek Poplawski on 5 Jan 2010 18:30 On Tue, Jan 05, 2010 at 06:16:58PM -0500, Michael Breuer wrote: > On 1/5/2010 6:07 PM, Jarek Poplawski wrote: > >David Miller wrote, On 12/27/2009 05:11 AM: > > > >>From: David Miller<davem(a)davemloft.net> > >>Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST) > >> > >>>From: Stephen Hemminger<shemminger(a)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. I'm not sure about David's (except do it first ;-), but try my patch with CONFIG_PACKET_MMAP on; if there is no change, CONFIG_PACKET_MMAP off, without any patches. Thanks, Jarek P. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Michael Breuer on 5 Jan 2010 21:40 On 1/5/2010 6:07 PM, Jarek Poplawski wrote: > David Miller wrote, On 12/27/2009 05:11 AM: > > >> From: David Miller<davem(a)davemloft.net> >> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST) >> >> >>> From: Stephen Hemminger<shemminger(a)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? > > 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(a)linux-foundation.org> > > Reported-by: Michael Breuer<mbreuer(a)majjas.com> > Signed-off-by: Jarek Poplawski<jarkao2(a)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); > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > This patch solves the original reported oops - as did Steven's patch of 12/26: [PATCH] sky2: make sure ethernet header is in transmit skb (I ran without Steven's patch and with this patch). Oddly, with this patch vs. Steven's - I'm getting software interrupt errors sporadically while not under load - with Steven's I get the frequently while under load (as per nethogs). For example: Jan 5 21:29:00 mail kernel: sky2 0000:06:00.0: error interrupt status=0x40000008 Jan 5 21:29:00 mail kernel: sky2 software interrupt status 0x40000008 Jan 5 21:29:00 mail kernel: sky2 Tx ring pending=124...125 report=125 done=125 Jan 5 21:29:00 mail kernel: 124: 0xb38de0be(5374) Jan 5 21:29:00 mail kernel: sky2 0000:06:00.0: error interrupt status=0x8 Jan 5 21:29:00 mail kernel: sky2 software interrupt status 0x8 Jan 5 21:29:00 mail kernel: sky2 Tx ring pending=126...127 report=126 done=127 Jan 5 21:29:00 mail kernel: 126: 0xb38d80be(9014) I also believe (can't prove yet) that my load test is slower with this patch vs. the sky2 patch. Is it possible that this patch is causing the transmission to momentarily halt such that the overall utilization appears low at the time I see the interrupt errors, vs. the other patch which perhaps doesn't interrupt the traffic flow quite so much? I haven't run yet with CONFIG_PACKET_MMAP disabled. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jarek Poplawski on 6 Jan 2010 02:30 On Tue, Jan 05, 2010 at 09:36:28PM -0500, Michael Breuer wrote: > On 1/5/2010 6:07 PM, Jarek Poplawski wrote: >> David Miller wrote, On 12/27/2009 05:11 AM: >> >> >>> From: David Miller<davem(a)davemloft.net> >>> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST) >>> >>> >>>> From: Stephen Hemminger<shemminger(a)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? >> >> 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(a)linux-foundation.org> >> >> Reported-by: Michael Breuer<mbreuer(a)majjas.com> >> Signed-off-by: Jarek Poplawski<jarkao2(a)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); >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo(a)vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ >> > This patch solves the original reported oops - as did Steven's patch of > 12/26: [PATCH] sky2: make sure ethernet header is in transmit skb (I ran > without Steven's patch and with this patch). > > Oddly, with this patch vs. Steven's - I'm getting software interrupt > errors sporadically while not under load - with Steven's I get the > frequently while under load (as per nethogs). For example: > Jan 5 21:29:00 mail kernel: sky2 0000:06:00.0: error interrupt > status=0x40000008 > Jan 5 21:29:00 mail kernel: sky2 software interrupt status 0x40000008 > Jan 5 21:29:00 mail kernel: sky2 Tx ring pending=124...125 report=125 > done=125 > Jan 5 21:29:00 mail kernel: 124: 0xb38de0be(5374) > Jan 5 21:29:00 mail kernel: sky2 0000:06:00.0: error interrupt status=0x8 > Jan 5 21:29:00 mail kernel: sky2 software interrupt status 0x8 > Jan 5 21:29:00 mail kernel: sky2 Tx ring pending=126...127 report=126 > done=127 > Jan 5 21:29:00 mail kernel: 126: 0xb38d80be(9014) > > I also believe (can't prove yet) that my load test is slower with this > patch vs. the sky2 patch. > > Is it possible that this patch is causing the transmission to > momentarily halt such that the overall utilization appears low at the > time I see the interrupt errors, vs. the other patch which perhaps > doesn't interrupt the traffic flow quite so much? Yes, without this patch xmit could be stopped earlier on some kind of errors, with retransmit of the last message possible. On the other hand, other dev_queue_xmit() (negative) errors, are ignored. So this place could be still improved by adding proper err handling (or removing getting err return from dev_queue_xmit() at all). Anyway, I think this patch should be a safe proposal for stable. If so, David, please add Michael's "Tested-by". > > I haven't run yet with CONFIG_PACKET_MMAP disabled. > This should behave similarly as MMAP with this patch or maybe even better in case of errors. Thanks, Jarek P. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Stephen Hemminger on 6 Jan 2010 10:00
On Wed, 6 Jan 2010 09:15:32 +0000 Jarek Poplawski <jarkao2(a)gmail.com> wrote: > tpacket_snd() can change and kfree an skb after dev_queue_xmit(), > which is illegal. > > With debugging by: Stephen Hemminger <shemminger(a)linux-foundation.org> > > Reported-by: Michael Breuer <mbreuer(a)majjas.com> > Signed-off-by: Jarek Poplawski <jarkao2(a)gmail.com> Good catch. Acked-by: Stephen Hemminger <shemminger(a)vyatta.com> -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |