[PATCH net-next] drivers/net/wan/hdlc_fr: Move the skb_headroom check out of fr_hard_header

[PATCH net-next] drivers/net/wan/hdlc_fr: Move the skb_headroom check out of fr_hard_header

[Xie He]

Move the skb_headroom check out of fr_hard_header and into pvc_xmit.
This has two benefits:

1. Originally we only do this check for skbs sent by users on Ethernet-
emulating PVC devices. After the change we do this check for skbs sent on
normal PVC devices, too.
(Also add a comment to make it clear that this is only a protection
against upper layers that don't take dev->needed_headroom into account.
Such upper layers should be rare and I believe they should be fixed.)

2. After the change we can simplify the parameter list of fr_hard_header.
We no longer need to use a pointer to pointers (skb_p) because we no
longer need to replace the skb inside fr_hard_header.

Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 4dfdbca54296..409e5a7ad8e2 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -271,10 +271,8 @@ static inline struct net_device **get_dev_p(struct pvc_device *pvc,
 }
 
 
-static int fr_hard_header(struct sk_buff **skb_p, u16 dlci)
+static int fr_hard_header(struct sk_buff *skb, u16 dlci)
 {
-	struct sk_buff *skb = *skb_p;
-
 	if (!skb->dev) { /* Control packets */
 		switch (dlci) {
 		case LMI_CCITT_ANSI_DLCI:
@@ -316,13 +314,6 @@ static int fr_hard_header(struct sk_buff **skb_p, u16 dlci)
 		}
 
 	} else if (skb->dev->type == ARPHRD_ETHER) {
-		if (skb_headroom(skb) < 10) {
-			struct sk_buff *skb2 = skb_realloc_headroom(skb, 10);
-			if (!skb2)
-				return -ENOBUFS;
-			dev_kfree_skb(skb);
-			skb = *skb_p = skb2;
-		}
 		skb_push(skb, 10);
 		skb->data[3] = FR_PAD;
 		skb->data[4] = NLPID_SNAP;
@@ -429,8 +420,21 @@ static netdev_tx_t pvc_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 
+	/* We already requested the header space with dev->needed_headroom.
+	 * So this is just a protection in case the upper layer didn't take
+	 * dev->needed_headroom into consideration.
+	 */
+	if (skb_headroom(skb) < 10) {
+		struct sk_buff *skb2 = skb_realloc_headroom(skb, 10);
+
+		if (!skb2)
+			goto drop;
+		dev_kfree_skb(skb);
+		skb = skb2;
+	}
+
 	skb->dev = dev;
-	if (fr_hard_header(&skb, pvc->dlci))
+	if (fr_hard_header(skb, pvc->dlci))
 		goto drop;
 
 	dev->stats.tx_bytes += skb->len;
@@ -498,9 +502,9 @@ static void fr_lmi_send(struct net_device *dev, int fullrep)
 	memset(skb->data, 0, len);
 	skb_reserve(skb, 4);
 	if (lmi == LMI_CISCO) {
-		fr_hard_header(&skb, LMI_CISCO_DLCI);
+		fr_hard_header(skb, LMI_CISCO_DLCI);
 	} else {
-		fr_hard_header(&skb, LMI_CCITT_ANSI_DLCI);
+		fr_hard_header(skb, LMI_CCITT_ANSI_DLCI);
 	}
 	data = skb_tail_pointer(skb);
 	data[i++] = LMI_CALLREF;
-- 
2.25.1

Re: [PATCH net-next] drivers/net/wan/hdlc_fr: Move the skb_headroom check out of fr_hard_header

[Jakub Kicinski]

On Wed,  7 Oct 2020 11:32:03 -0700 Xie He wrote:
> Move the skb_headroom check out of fr_hard_header and into pvc_xmit.
> This has two benefits:
> 
> 1. Originally we only do this check for skbs sent by users on Ethernet-
> emulating PVC devices. After the change we do this check for skbs sent on
> normal PVC devices, too.
> (Also add a comment to make it clear that this is only a protection
> against upper layers that don't take dev->needed_headroom into account.
> Such upper layers should be rare and I believe they should be fixed.)
> 
> 2. After the change we can simplify the parameter list of fr_hard_header.
> We no longer need to use a pointer to pointers (skb_p) because we no
> longer need to replace the skb inside fr_hard_header.
> 
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Applied, thanks!