| Message ID | 5593E497.40804@nokia.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Alexander Sverdlin <alexander.sverdlin@nokia.com> Date: Wed, 1 Jul 2015 15:01:11 +0200 > It's not allowed to assign data pointer of skbuff directly, this makes no sense > if the assigned pointer is the very same as already existing one, or it brakes > all the pointer arithmetics in all other cases. We cannot do better as just > compare them and report BUG() in case of mismatch. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> BUG takes the entire machine out, which is worse than corrupting the skb->data If you really want to assert this condition, do it in a way that doesn't kill the entire machine. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, On 02/07/15 21:12, ext David Miller wrote: >> It's not allowed to assign data pointer of skbuff directly, this makes no sense >> > if the assigned pointer is the very same as already existing one, or it brakes >> > all the pointer arithmetics in all other cases. We cannot do better as just >> > compare them and report BUG() in case of mismatch. >> > >> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > BUG takes the entire machine out, which is worse than corrupting the > skb->data > > If you really want to assert this condition, do it in a way that > doesn't kill the entire machine. In fact, the machine goes down, some milliseconds later, but because of the following inconsistencies, which are misleading. The function has no way to signal an error and this line of code is simply wrong. To prevent others from copying this error, we can simply delete it. Would it be fine from your PoV?
diff --git a/drivers/net/rionet.c b/drivers/net/rionet.c index dac7a0d..34c27b8 100644 --- a/drivers/net/rionet.c +++ b/drivers/net/rionet.c @@ -104,7 +104,8 @@ static int rionet_rx_clean(struct net_device *ndev) if (!(data = rio_get_inb_message(rnet->mport, RIONET_MAILBOX))) break; - rnet->rx_skb[i]->data = data; + if (rnet->rx_skb[i]->data != data) + BUG(); skb_put(rnet->rx_skb[i], RIO_MAX_MSG_SIZE); rnet->rx_skb[i]->protocol = eth_type_trans(rnet->rx_skb[i], ndev);
It's not allowed to assign data pointer of skbuff directly, this makes no sense if the assigned pointer is the very same as already existing one, or it brakes all the pointer arithmetics in all other cases. We cannot do better as just compare them and report BUG() in case of mismatch. Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> --- We came across this problem developing new code for Octeon2 RAPIDIO. For the last 10 years since original commit of the code this assignment did nothing as the pointers were always same. But the bug in the new code discovered this one. So better do BUG() immediately here, this would prevent longer debugging of the following skbuff corruption. drivers/net/rionet.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html