Re: [PATCH 3/3] Fix use of skb after netif_rx
From: Julia Lawall <[EMAIL PROTECTED]> Date: Sun, 9 Dec 2007 21:05:30 +0100 (CET) > From: Julia Lawall <[EMAIL PROTECTED]> > > Recently, Wang Chen submitted a patch > (d30f53aeb31d453a5230f526bea592af07944564) to move a call to netif_rx(skb) > after a subsequent reference to skb, because netif_rx may call kfree_skb on > its argument. netif_rx_ni calls netif_rx, so the same problem occurs in > the files below. > > I have left the updating of dev->last_rx after the calls to netif_rx_ni > because it seems time dependent, but moved the other field updates before. > > This was found using the following semantic match. > (http://www.emn.fr/x-info/coccinelle/) ... > Signed-off-by: Julia Lawall <[EMAIL PROTECTED]> Applied. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
From: Julia Lawall [EMAIL PROTECTED] Date: Sun, 9 Dec 2007 21:05:30 +0100 (CET) From: Julia Lawall [EMAIL PROTECTED] Recently, Wang Chen submitted a patch (d30f53aeb31d453a5230f526bea592af07944564) to move a call to netif_rx(skb) after a subsequent reference to skb, because netif_rx may call kfree_skb on its argument. netif_rx_ni calls netif_rx, so the same problem occurs in the files below. I have left the updating of dev-last_rx after the calls to netif_rx_ni because it seems time dependent, but moved the other field updates before. This was found using the following semantic match. (http://www.emn.fr/x-info/coccinelle/) ... Signed-off-by: Julia Lawall [EMAIL PROTECTED] Applied. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
Julia Lawall said the following on 2007-12-10 15:18: >> Julia, seems that your semantic patch misses following place. >> >> drivers/s390/net/qeth_main.c:2733 >> ... >> #endif >> rxrc = netif_rx(skb); >> card->dev->last_rx = jiffies; >> card->stats.rx_packets++; >> card->stats.rx_bytes += skb->len; >> ... > > Actually, I found this one as well, but I wasn't sure what to do with it. > This one is a bit more complicated because the line with the call to > netif_rx is in an else branch if the #ifdef above is taken. So I wasn't > sure what would be the best way to solve the problem in this case. > > Perhaps the solution would be just to save the value of the len field > in a local variable in this case, as you proposed in your original patch. > I agree. BTW, please send driver patch to Jeff Garzik <[EMAIL PROTECTED]> and cc to [EMAIL PROTECTED] -- WCN -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
> > // > > > > diff a/drivers/s390/net/ctcmain.c b/drivers/s390/net/ctcmain.c > > diff a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c > > Julia, seems that your semantic patch misses following place. > > drivers/s390/net/qeth_main.c:2733 > ... > #endif > rxrc = netif_rx(skb); > card->dev->last_rx = jiffies; > card->stats.rx_packets++; > card->stats.rx_bytes += skb->len; > ... Actually, I found this one as well, but I wasn't sure what to do with it. This one is a bit more complicated because the line with the call to netif_rx is in an else branch if the #ifdef above is taken. So I wasn't sure what would be the best way to solve the problem in this case. Perhaps the solution would be just to save the value of the len field in a local variable in this case, as you proposed in your original patch. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
Julia Lawall said the following on 2007-12-10 4:05: > From: Julia Lawall <[EMAIL PROTECTED]> > // > @@ > expression skb, e,e1; > @@ > > ( > netif_rx(skb); > | > netif_rx_ni(skb); > ) > ... when != skb = e > ( > skb = e1 > | > * skb > ) > // > > diff a/drivers/s390/net/ctcmain.c b/drivers/s390/net/ctcmain.c > diff a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c Julia, seems that your semantic patch misses following place. drivers/s390/net/qeth_main.c:2733 ... #endif rxrc = netif_rx(skb); card->dev->last_rx = jiffies; card->stats.rx_packets++; card->stats.rx_bytes += skb->len; ... -- WCN -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] Fix use of skb after netif_rx
From: Julia Lawall <[EMAIL PROTECTED]>
Recently, Wang Chen submitted a patch
(d30f53aeb31d453a5230f526bea592af07944564) to move a call to netif_rx(skb)
after a subsequent reference to skb, because netif_rx may call kfree_skb on
its argument. netif_rx_ni calls netif_rx, so the same problem occurs in
the files below.
I have left the updating of dev->last_rx after the calls to netif_rx_ni
because it seems time dependent, but moved the other field updates before.
This was found using the following semantic match.
(http://www.emn.fr/x-info/coccinelle/)
//
@@
expression skb, e,e1;
@@
(
netif_rx(skb);
|
netif_rx_ni(skb);
)
... when != skb = e
(
skb = e1
|
* skb
)
//
Signed-off-by: Julia Lawall <[EMAIL PROTECTED]>
---
diff a/drivers/s390/net/ctcmain.c b/drivers/s390/net/ctcmain.c
--- a/drivers/s390/net/ctcmain.c2007-12-05 09:21:56.0 +0100
+++ b/drivers/s390/net/ctcmain.c2007-12-05 19:03:40.0 +0100
@@ -478,14 +478,14 @@ ctc_unpack_skb(struct channel *ch, struc
skb->dev = pskb->dev;
skb->protocol = pskb->protocol;
pskb->ip_summed = CHECKSUM_UNNECESSARY;
- netif_rx_ni(skb);
/**
-* Successful rx; reset logflags
+* reset logflags
*/
ch->logflags = 0;
- dev->last_rx = jiffies;
privptr->stats.rx_packets++;
privptr->stats.rx_bytes += skb->len;
+ netif_rx_ni(skb);
+ dev->last_rx = jiffies;
if (len > 0) {
skb_pull(pskb, header->length);
if (skb_tailroom(pskb) < LL_HEADER_LENGTH) {
diff a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
--- a/drivers/s390/net/netiucv.c2007-10-22 11:25:20.0 +0200
+++ b/drivers/s390/net/netiucv.c2007-12-05 19:03:10.0 +0100
@@ -639,14 +639,14 @@ static void netiucv_unpack_skb(struct iu
skb->dev = pskb->dev;
skb->protocol = pskb->protocol;
pskb->ip_summed = CHECKSUM_UNNECESSARY;
+ privptr->stats.rx_packets++;
+ privptr->stats.rx_bytes += skb->len;
/*
* Since receiving is always initiated from a tasklet (in
iucv.c),
* we must use netif_rx_ni() instead of netif_rx()
*/
netif_rx_ni(skb);
dev->last_rx = jiffies;
- privptr->stats.rx_packets++;
- privptr->stats.rx_bytes += skb->len;
skb_pull(pskb, header->next);
skb_put(pskb, NETIUCV_HDRLEN);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] Fix use of skb after netif_rx
From: Julia Lawall [EMAIL PROTECTED]
Recently, Wang Chen submitted a patch
(d30f53aeb31d453a5230f526bea592af07944564) to move a call to netif_rx(skb)
after a subsequent reference to skb, because netif_rx may call kfree_skb on
its argument. netif_rx_ni calls netif_rx, so the same problem occurs in
the files below.
I have left the updating of dev-last_rx after the calls to netif_rx_ni
because it seems time dependent, but moved the other field updates before.
This was found using the following semantic match.
(http://www.emn.fr/x-info/coccinelle/)
// smpl
@@
expression skb, e,e1;
@@
(
netif_rx(skb);
|
netif_rx_ni(skb);
)
... when != skb = e
(
skb = e1
|
* skb
)
// /smpl
Signed-off-by: Julia Lawall [EMAIL PROTECTED]
---
diff a/drivers/s390/net/ctcmain.c b/drivers/s390/net/ctcmain.c
--- a/drivers/s390/net/ctcmain.c2007-12-05 09:21:56.0 +0100
+++ b/drivers/s390/net/ctcmain.c2007-12-05 19:03:40.0 +0100
@@ -478,14 +478,14 @@ ctc_unpack_skb(struct channel *ch, struc
skb-dev = pskb-dev;
skb-protocol = pskb-protocol;
pskb-ip_summed = CHECKSUM_UNNECESSARY;
- netif_rx_ni(skb);
/**
-* Successful rx; reset logflags
+* reset logflags
*/
ch-logflags = 0;
- dev-last_rx = jiffies;
privptr-stats.rx_packets++;
privptr-stats.rx_bytes += skb-len;
+ netif_rx_ni(skb);
+ dev-last_rx = jiffies;
if (len 0) {
skb_pull(pskb, header-length);
if (skb_tailroom(pskb) LL_HEADER_LENGTH) {
diff a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
--- a/drivers/s390/net/netiucv.c2007-10-22 11:25:20.0 +0200
+++ b/drivers/s390/net/netiucv.c2007-12-05 19:03:10.0 +0100
@@ -639,14 +639,14 @@ static void netiucv_unpack_skb(struct iu
skb-dev = pskb-dev;
skb-protocol = pskb-protocol;
pskb-ip_summed = CHECKSUM_UNNECESSARY;
+ privptr-stats.rx_packets++;
+ privptr-stats.rx_bytes += skb-len;
/*
* Since receiving is always initiated from a tasklet (in
iucv.c),
* we must use netif_rx_ni() instead of netif_rx()
*/
netif_rx_ni(skb);
dev-last_rx = jiffies;
- privptr-stats.rx_packets++;
- privptr-stats.rx_bytes += skb-len;
skb_pull(pskb, header-next);
skb_put(pskb, NETIUCV_HDRLEN);
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
Julia Lawall said the following on 2007-12-10 4:05: From: Julia Lawall [EMAIL PROTECTED] // smpl @@ expression skb, e,e1; @@ ( netif_rx(skb); | netif_rx_ni(skb); ) ... when != skb = e ( skb = e1 | * skb ) // /smpl diff a/drivers/s390/net/ctcmain.c b/drivers/s390/net/ctcmain.c diff a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c Julia, seems that your semantic patch misses following place. drivers/s390/net/qeth_main.c:2733 ... #endif rxrc = netif_rx(skb); card-dev-last_rx = jiffies; card-stats.rx_packets++; card-stats.rx_bytes += skb-len; ... -- WCN -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
// /smpl diff a/drivers/s390/net/ctcmain.c b/drivers/s390/net/ctcmain.c diff a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c Julia, seems that your semantic patch misses following place. drivers/s390/net/qeth_main.c:2733 ... #endif rxrc = netif_rx(skb); card-dev-last_rx = jiffies; card-stats.rx_packets++; card-stats.rx_bytes += skb-len; ... Actually, I found this one as well, but I wasn't sure what to do with it. This one is a bit more complicated because the line with the call to netif_rx is in an else branch if the #ifdef above is taken. So I wasn't sure what would be the best way to solve the problem in this case. Perhaps the solution would be just to save the value of the len field in a local variable in this case, as you proposed in your original patch. julia -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Fix use of skb after netif_rx
Julia Lawall said the following on 2007-12-10 15:18: Julia, seems that your semantic patch misses following place. drivers/s390/net/qeth_main.c:2733 ... #endif rxrc = netif_rx(skb); card-dev-last_rx = jiffies; card-stats.rx_packets++; card-stats.rx_bytes += skb-len; ... Actually, I found this one as well, but I wasn't sure what to do with it. This one is a bit more complicated because the line with the call to netif_rx is in an else branch if the #ifdef above is taken. So I wasn't sure what would be the best way to solve the problem in this case. Perhaps the solution would be just to save the value of the len field in a local variable in this case, as you proposed in your original patch. I agree. BTW, please send driver patch to Jeff Garzik [EMAIL PROTECTED] and cc to [EMAIL PROTECTED] -- WCN -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
