| Message ID | 20201004004619.291065-1-xie.he.0141@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] drivers/net/wan: lapb: Replace the skb->len checks with pskb_may_pull | expand |
From: Xie He <xie.he.0141@gmail.com> Date: Sat, 3 Oct 2020 17:46:19 -0700 > The purpose of these skb->len checks in these drivers is to ensure that > there is a header in the skb available to be read and pulled. > > However, we already have the pskb_may_pull function for this purpose. > > The pskb_may_pull function also correctly handles non-linear skbs. > > (Also delete the word "check" in the comments because pskb_may_pull may > do things other than simple checks in the case of non-linear skbs.) > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Cc: Martin Schiller <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> This is excessive. On transmit the header will be in the linear area, especially if it is only one byte in size. You're adding a lot of extra checks, function calls, etc. which are frankly unnecessary. I'm not applying this unless you can prove that a non-linear single header byte is possible in these code paths. And you'll need to add such proof to your commit message. Thank you.
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c index f52b9fed0593..891a7a918b29 100644 --- a/drivers/net/wan/hdlc_x25.c +++ b/drivers/net/wan/hdlc_x25.c @@ -108,9 +108,9 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev) int result; /* There should be a pseudo header of 1 byte added by upper layers. - * Check to make sure it is there before reading it. + * Make sure it is there before reading it. */ - if (skb->len < 1) { + if (!pskb_may_pull(skb, 1)) { kfree_skb(skb); return NETDEV_TX_OK; } diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c index b6be2454b8bd..e0cf692357d6 100644 --- a/drivers/net/wan/lapbether.c +++ b/drivers/net/wan/lapbether.c @@ -158,9 +158,9 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff *skb, goto drop; /* There should be a pseudo header of 1 byte added by upper layers. - * Check to make sure it is there before reading it. + * Make sure it is there before reading it. */ - if (skb->len < 1) + if (!pskb_may_pull(skb, 1)) goto drop; switch (skb->data[0]) { diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c index c418767a890a..b935a3e1d5f6 100644 --- a/drivers/net/wan/x25_asy.c +++ b/drivers/net/wan/x25_asy.c @@ -308,9 +308,9 @@ static netdev_tx_t x25_asy_xmit(struct sk_buff *skb, } /* There should be a pseudo header of 1 byte added by upper layers. - * Check to make sure it is there before reading it. + * Make sure it is there before reading it. */ - if (skb->len < 1) { + if (!pskb_may_pull(skb, 1)) { kfree_skb(skb); return NETDEV_TX_OK; }
The purpose of these skb->len checks in these drivers is to ensure that there is a header in the skb available to be read and pulled. However, we already have the pskb_may_pull function for this purpose. The pskb_may_pull function also correctly handles non-linear skbs. (Also delete the word "check" in the comments because pskb_may_pull may do things other than simple checks in the case of non-linear skbs.) Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Cc: Martin Schiller <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- drivers/net/wan/hdlc_x25.c | 4 ++-- drivers/net/wan/lapbether.c | 4 ++-- drivers/net/wan/x25_asy.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)