net: dsa: tag_qca: QCA tag and headroom size?

Question Per Förlin 2 participants 4 messages Feb 12, 2020 – Feb 13, 2020

Subsystems (4)

Networking Network drivers Networking DSA Qualcomm Atheros QCA8K DSA switch driver

lore

Share & tools

lore

Per Förlin

author

2020-02-12 09:57 UTC #0 lore raw

Hi,

---
 net/dsa/tag_qca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/dsa/tag_qca.c +0 -1

index c8a128c9e5e0..70db7c909f74 100644
@@ -33,7 +33,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	u16 *phdr, hdr;
 
-	if (skb_cow_head(skb, 0) < 0)

Quoted 1 line

Is it really safe to assume there is enough headroom for the QCA tag?

+ if (skb_cow_head(skb, QCA_HDR_LEN) < 0)

Quoted 1 line

My proposal. Specify QCA tag size to make sure there is headroom.

return NULL;

skb_push(skb, QCA_HDR_LEN);

Signature · 1 line

2.11.0

Andrew Lunn

maintainer

2020-02-12 20:23 UTC #1 lore raw

Quoted 20 lines · Per Förlin

Hi,
---
 net/dsa/tag_qca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/dsa/tag_qca.c +0 -1
index c8a128c9e5e0..70db7c909f74 100644
@@ -33,7 +33,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	u16 *phdr, hdr;
 
-	if (skb_cow_head(skb, 0) < 0)
Quoted 1 line
Is it really safe to assume there is enough headroom for the QCA tag?
net/dsa/tag_qca.c +1 -0
+	if (skb_cow_head(skb, QCA_HDR_LEN) < 0)
Quoted 1 line
My proposal. Specify QCA tag size to make sure there is headroom.

net/dsa/tag_qca.c

Quoted 3 lines

net/dsa/tag_qca.c +0 -0

 		return NULL;
 
 	skb_push(skb, QCA_HDR_LEN);

Hi Per

Yes, your change looks correct. ar9331_tag_xmit() also seems to have the same problem.

Do you want to submit a patch?

Thanks

Andrew

Per Förlin

author

2020-02-13 05:24 UTC #2 lore raw

Quoted 9 lines

Quoted 9 lines
- if (skb_cow_head(skb, 0) < 0)
Quoted 1 line
Is it really safe to assume there is enough headroom for the QCA tag?
+ if (skb_cow_head(skb, QCA_HDR_LEN) < 0)
Quoted 1 line
My proposal. Specify QCA tag size to make sure there is headroom.
              return NULL;

Quoted 8 lines

Quoted 8 lines
      skb_push(skb, QCA_HDR_LEN);
Hi Per
Yes, your change looks correct. ar9331_tag_xmit() also seems to have the same problem.
Do you want to submit a patch?

Thanks for your response,
I can submit a patch on both drivers, however I only have hardware to perform an actual test on the QCA switch.

Andrew Lunn

maintainer

2020-02-13 12:53 UTC #3 lore raw

Quoted 13 lines · Per Förlin

Quoted 12 lines
Quoted 12 lines
- if (skb_cow_head(skb, 0) < 0)
Quoted 1 line
Is it really safe to assume there is enough headroom for the QCA tag?
+ if (skb_cow_head(skb, QCA_HDR_LEN) < 0)
Quoted 1 line
My proposal. Specify QCA tag size to make sure there is headroom.
              return NULL;

      skb_push(skb, QCA_HDR_LEN);
Hi Per

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Yes, your change looks correct. ar9331_tag_xmit() also seems to have the same problem.
Do you want to submit a patch?
Thanks for your response,
I can submit a patch on both drivers, however I only have hardware to perform an actual test on the QCA switch.

Hi Per

That is fine. This should be low risk. Cc: Oleksij Rempel <[email protected]> when you post the patch, so he can review and test it.

Thanks
Andrew