* [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
@ 2021-02-16 23:55 Linus Walleij
2021-02-17 1:00 ` patchwork-bot+netdevbpf
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Linus Walleij @ 2021-02-16 23:55 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, Mauri Sandberg, DENG Qingfang
Support also transmitting frames using the custom "8899 A"
4 byte tag.
Qingfang came up with the solution: we need to pad the
ethernet frame to 60 bytes using eth_skb_pad(), then the
switch will happily accept frames with custom tags.
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reported-by: DENG Qingfang <dqfext@gmail.com>
Fixes: efd7fe68f0c6 ("net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
net/dsa/tag_rtl4_a.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index 2646abe5a69e..c17d39b4a1a0 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -12,9 +12,7 @@
*
* The 2 bytes tag form a 16 bit big endian word. The exact
* meaning has been guessed from packet dumps from ingress
- * frames, as no working egress traffic has been available
- * we do not know the format of the egress tags or if they
- * are even supported.
+ * frames.
*/
#include <linux/etherdevice.h>
@@ -36,17 +34,34 @@
static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
struct net_device *dev)
{
- /*
- * Just let it pass thru, we don't know if it is possible
- * to tag a frame with the 0x8899 ethertype and direct it
- * to a specific port, all attempts at reverse-engineering have
- * ended up with the frames getting dropped.
- *
- * The VLAN set-up needs to restrict the frames to the right port.
- *
- * If you have documentation on the tagging format for RTL8366RB
- * (tag type A) then please contribute.
- */
+ struct dsa_port *dp = dsa_slave_to_port(dev);
+ u8 *tag;
+ u16 *p;
+ u16 out;
+
+ /* Pad out to at least 60 bytes */
+ if (unlikely(eth_skb_pad(skb)))
+ return NULL;
+ if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0)
+ return NULL;
+
+ netdev_dbg(dev, "add realtek tag to package to port %d\n",
+ dp->index);
+ skb_push(skb, RTL4_A_HDR_LEN);
+
+ memmove(skb->data, skb->data + RTL4_A_HDR_LEN, 2 * ETH_ALEN);
+ tag = skb->data + 2 * ETH_ALEN;
+
+ /* Set Ethertype */
+ p = (u16 *)tag;
+ *p = htons(RTL4_A_ETHERTYPE);
+
+ out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
+ /* The lower bits is the port numer */
+ out |= (u8)dp->index;
+ p = (u16 *)(tag + 2);
+ *p = htons(out);
+
return skb;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-16 23:55 [PATCH] net: dsa: tag_rtl4_a: Support also egress tags Linus Walleij
@ 2021-02-17 1:00 ` patchwork-bot+netdevbpf
2021-02-17 1:04 ` Florian Fainelli
2021-02-17 3:29 ` Andrew Lunn
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-02-17 1:00 UTC (permalink / raw)
To: Linus Walleij
Cc: andrew, vivien.didelot, f.fainelli, olteanv, davem, kuba, netdev,
sandberg, dqfext
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Wed, 17 Feb 2021 00:55:42 +0100 you wrote:
> Support also transmitting frames using the custom "8899 A"
> 4 byte tag.
>
> Qingfang came up with the solution: we need to pad the
> ethernet frame to 60 bytes using eth_skb_pad(), then the
> switch will happily accept frames with custom tags.
>
> [...]
Here is the summary with links:
- net: dsa: tag_rtl4_a: Support also egress tags
https://git.kernel.org/netdev/net-next/c/86dd9868b878
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-17 1:00 ` patchwork-bot+netdevbpf
@ 2021-02-17 1:04 ` Florian Fainelli
0 siblings, 0 replies; 11+ messages in thread
From: Florian Fainelli @ 2021-02-17 1:04 UTC (permalink / raw)
To: patchwork-bot+netdevbpf, Linus Walleij
Cc: andrew, vivien.didelot, olteanv, davem, kuba, netdev, sandberg, dqfext
On 2/16/2021 5:00 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
>
> This patch was applied to netdev/net-next.git (refs/heads/master):
>
> On Wed, 17 Feb 2021 00:55:42 +0100 you wrote:
>> Support also transmitting frames using the custom "8899 A"
>> 4 byte tag.
>>
>> Qingfang came up with the solution: we need to pad the
>> ethernet frame to 60 bytes using eth_skb_pad(), then the
>> switch will happily accept frames with custom tags.
>>
>> [...]
>
> Here is the summary with links:
> - net: dsa: tag_rtl4_a: Support also egress tags
> https://git.kernel.org/netdev/net-next/c/86dd9868b878
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
Woah not so fast, 1h05 minutes to review a patch, how are we supposed to
keep up?
--
Florian
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-16 23:55 [PATCH] net: dsa: tag_rtl4_a: Support also egress tags Linus Walleij
2021-02-17 1:00 ` patchwork-bot+netdevbpf
@ 2021-02-17 3:29 ` Andrew Lunn
2021-02-17 5:37 ` DENG Qingfang
2021-02-17 5:40 ` DENG Qingfang
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2021-02-17 3:29 UTC (permalink / raw)
To: Linus Walleij
Cc: Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg,
DENG Qingfang
On Wed, Feb 17, 2021 at 12:55:42AM +0100, Linus Walleij wrote:
> Support also transmitting frames using the custom "8899 A"
> 4 byte tag.
>
> Qingfang came up with the solution: we need to pad the
> ethernet frame to 60 bytes using eth_skb_pad(), then the
> switch will happily accept frames with custom tags.
>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Reported-by: DENG Qingfang <dqfext@gmail.com>
> Fixes: efd7fe68f0c6 ("net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> net/dsa/tag_rtl4_a.c | 43 +++++++++++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
> index 2646abe5a69e..c17d39b4a1a0 100644
> --- a/net/dsa/tag_rtl4_a.c
> +++ b/net/dsa/tag_rtl4_a.c
> @@ -12,9 +12,7 @@
> *
> * The 2 bytes tag form a 16 bit big endian word. The exact
> * meaning has been guessed from packet dumps from ingress
> - * frames, as no working egress traffic has been available
> - * we do not know the format of the egress tags or if they
> - * are even supported.
> + * frames.
> */
>
> #include <linux/etherdevice.h>
> @@ -36,17 +34,34 @@
> static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> - /*
> - * Just let it pass thru, we don't know if it is possible
> - * to tag a frame with the 0x8899 ethertype and direct it
> - * to a specific port, all attempts at reverse-engineering have
> - * ended up with the frames getting dropped.
> - *
> - * The VLAN set-up needs to restrict the frames to the right port.
> - *
> - * If you have documentation on the tagging format for RTL8366RB
> - * (tag type A) then please contribute.
> - */
> + struct dsa_port *dp = dsa_slave_to_port(dev);
> + u8 *tag;
> + u16 *p;
> + u16 out;
Reverse Christmass tree.
> +
> + /* Pad out to at least 60 bytes */
> + if (unlikely(eth_skb_pad(skb)))
> + return NULL;
The core will do the padding for you. Turn on .tail_tag in
dsa_device_ops.
> + if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0)
> + return NULL;
> +
> + netdev_dbg(dev, "add realtek tag to package to port %d\n",
> + dp->index);
You can remove this, now that it works.
> + skb_push(skb, RTL4_A_HDR_LEN);
> +
> + memmove(skb->data, skb->data + RTL4_A_HDR_LEN, 2 * ETH_ALEN);
> + tag = skb->data + 2 * ETH_ALEN;
> +
> + /* Set Ethertype */
> + p = (u16 *)tag;
> + *p = htons(RTL4_A_ETHERTYPE);
> +
> + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
Do we know what bit 9 means?
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-17 3:29 ` Andrew Lunn
@ 2021-02-17 5:37 ` DENG Qingfang
2021-02-17 11:41 ` Vladimir Oltean
0 siblings, 1 reply; 11+ messages in thread
From: DENG Qingfang @ 2021-02-17 5:37 UTC (permalink / raw)
To: Andrew Lunn
Cc: Linus Walleij, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg
On Wed, Feb 17, 2021 at 11:29 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > + /* Pad out to at least 60 bytes */
> > + if (unlikely(eth_skb_pad(skb)))
> > + return NULL;
>
> The core will do the padding for you. Turn on .tail_tag in
> dsa_device_ops.
>
Please don't. .tail_tag does far more than just padding.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-16 23:55 [PATCH] net: dsa: tag_rtl4_a: Support also egress tags Linus Walleij
2021-02-17 1:00 ` patchwork-bot+netdevbpf
2021-02-17 3:29 ` Andrew Lunn
@ 2021-02-17 5:40 ` DENG Qingfang
2021-02-17 11:33 ` Vladimir Oltean
2021-02-17 12:38 ` DENG Qingfang
4 siblings, 0 replies; 11+ messages in thread
From: DENG Qingfang @ 2021-02-17 5:40 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg
On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> + if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0)
> + return NULL;
skb_cow_head is unnecessary here. DSA core will do it for you.
> +
> + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> + /* The lower bits is the port numer */
Typo?
> + out |= (u8)dp->index;
> + p = (u16 *)(tag + 2);
> + *p = htons(out);
> +
> return skb;
> }
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-16 23:55 [PATCH] net: dsa: tag_rtl4_a: Support also egress tags Linus Walleij
` (2 preceding siblings ...)
2021-02-17 5:40 ` DENG Qingfang
@ 2021-02-17 11:33 ` Vladimir Oltean
2021-02-17 12:38 ` DENG Qingfang
4 siblings, 0 replies; 11+ messages in thread
From: Vladimir Oltean @ 2021-02-17 11:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, Mauri Sandberg, DENG Qingfang
On Wed, Feb 17, 2021 at 12:55:42AM +0100, Linus Walleij wrote:
> Support also transmitting frames using the custom "8899 A"
> 4 byte tag.
>
> Qingfang came up with the solution: we need to pad the
> ethernet frame to 60 bytes using eth_skb_pad(), then the
> switch will happily accept frames with custom tags.
I think it's pretty frustrating to realize that 'it didn't werk' because
you only tested with ping.
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Reported-by: DENG Qingfang <dqfext@gmail.com>
Reported-by seems like a bit of an understatement. Suggested-by maybe?
> Fixes: efd7fe68f0c6 ("net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> net/dsa/tag_rtl4_a.c | 43 +++++++++++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
> index 2646abe5a69e..c17d39b4a1a0 100644
> --- a/net/dsa/tag_rtl4_a.c
> +++ b/net/dsa/tag_rtl4_a.c
> @@ -12,9 +12,7 @@
> *
> * The 2 bytes tag form a 16 bit big endian word. The exact
> * meaning has been guessed from packet dumps from ingress
> - * frames, as no working egress traffic has been available
> - * we do not know the format of the egress tags or if they
> - * are even supported.
> + * frames.
> */
>
> #include <linux/etherdevice.h>
> @@ -36,17 +34,34 @@
> static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> - /*
> - * Just let it pass thru, we don't know if it is possible
> - * to tag a frame with the 0x8899 ethertype and direct it
> - * to a specific port, all attempts at reverse-engineering have
> - * ended up with the frames getting dropped.
> - *
> - * The VLAN set-up needs to restrict the frames to the right port.
> - *
> - * If you have documentation on the tagging format for RTL8366RB
> - * (tag type A) then please contribute.
> - */
> + struct dsa_port *dp = dsa_slave_to_port(dev);
> + u8 *tag;
> + u16 *p;
> + u16 out;
> +
> + /* Pad out to at least 60 bytes */
> + if (unlikely(eth_skb_pad(skb)))
> + return NULL;
> + if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0)
> + return NULL;
> +
> + netdev_dbg(dev, "add realtek tag to package to port %d\n",
> + dp->index);
You should probably remove any sort of printing from the hot path.
> + skb_push(skb, RTL4_A_HDR_LEN);
> +
> + memmove(skb->data, skb->data + RTL4_A_HDR_LEN, 2 * ETH_ALEN);
> + tag = skb->data + 2 * ETH_ALEN;
> +
> + /* Set Ethertype */
> + p = (u16 *)tag;
> + *p = htons(RTL4_A_ETHERTYPE);
> +
> + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> + /* The lower bits is the port numer */
> + out |= (u8)dp->index;
> + p = (u16 *)(tag + 2);
> + *p = htons(out);
> +
> return skb;
> }
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-17 5:37 ` DENG Qingfang
@ 2021-02-17 11:41 ` Vladimir Oltean
0 siblings, 0 replies; 11+ messages in thread
From: Vladimir Oltean @ 2021-02-17 11:41 UTC (permalink / raw)
To: DENG Qingfang
Cc: Andrew Lunn, Linus Walleij, Vivien Didelot, Florian Fainelli,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg
On Wed, Feb 17, 2021 at 01:37:21PM +0800, DENG Qingfang wrote:
> On Wed, Feb 17, 2021 at 11:29 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > + /* Pad out to at least 60 bytes */
> > > + if (unlikely(eth_skb_pad(skb)))
> > > + return NULL;
> >
> > The core will do the padding for you. Turn on .tail_tag in
> > dsa_device_ops.
> >
>
> Please don't. .tail_tag does far more than just padding.
Qingfang is right, this is an EtherType tagging protocol not a tail tagger.
Note that there was an interesting discussion with Florian and Kuba
around the Broadcom switches that require padding for short frames too:
https://patchwork.kernel.org/project/netdevbpf/patch/20201101191620.589272-10-vladimir.oltean@nxp.com/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-16 23:55 [PATCH] net: dsa: tag_rtl4_a: Support also egress tags Linus Walleij
` (3 preceding siblings ...)
2021-02-17 11:33 ` Vladimir Oltean
@ 2021-02-17 12:38 ` DENG Qingfang
2021-02-17 12:42 ` Vladimir Oltean
4 siblings, 1 reply; 11+ messages in thread
From: DENG Qingfang @ 2021-02-17 12:38 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg
On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> +
> + /* Pad out to at least 60 bytes */
> + if (unlikely(eth_skb_pad(skb)))
> + return NULL;
I just found that this will cause double free (eth_skb_pad will free
the skb if allocation fails, and dsa_slave_xmit will still try to free
it because it returns NULL.
Replace eth_skb_pad(skb) with __skb_put_padto(skb, ETH_ZLEN, false) to
avoid that.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-17 12:38 ` DENG Qingfang
@ 2021-02-17 12:42 ` Vladimir Oltean
2021-02-17 14:19 ` Heiner Kallweit
0 siblings, 1 reply; 11+ messages in thread
From: Vladimir Oltean @ 2021-02-17 12:42 UTC (permalink / raw)
To: DENG Qingfang
Cc: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg
On Wed, Feb 17, 2021 at 08:38:30PM +0800, DENG Qingfang wrote:
> On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > +
> > + /* Pad out to at least 60 bytes */
> > + if (unlikely(eth_skb_pad(skb)))
> > + return NULL;
>
> I just found that this will cause double free (eth_skb_pad will free
> the skb if allocation fails, and dsa_slave_xmit will still try to free
> it because it returns NULL.
> Replace eth_skb_pad(skb) with __skb_put_padto(skb, ETH_ZLEN, false) to
> avoid that.
Good catch, Qingfang.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: tag_rtl4_a: Support also egress tags
2021-02-17 12:42 ` Vladimir Oltean
@ 2021-02-17 14:19 ` Heiner Kallweit
0 siblings, 0 replies; 11+ messages in thread
From: Heiner Kallweit @ 2021-02-17 14:19 UTC (permalink / raw)
To: Vladimir Oltean, DENG Qingfang
Cc: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg
On 17.02.2021 13:42, Vladimir Oltean wrote:
> On Wed, Feb 17, 2021 at 08:38:30PM +0800, DENG Qingfang wrote:
>> On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>> +
>>> + /* Pad out to at least 60 bytes */
>>> + if (unlikely(eth_skb_pad(skb)))
>>> + return NULL;
>>
>> I just found that this will cause double free (eth_skb_pad will free
>> the skb if allocation fails, and dsa_slave_xmit will still try to free
>> it because it returns NULL.
>> Replace eth_skb_pad(skb) with __skb_put_padto(skb, ETH_ZLEN, false) to
>> avoid that.
>
That's really a pitfall. I had to do the same in r8169:
cc6528bc9a0c ("r8169: fix potential skb double free in an error path")
> Good catch, Qingfang.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-17 14:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-16 23:55 [PATCH] net: dsa: tag_rtl4_a: Support also egress tags Linus Walleij
2021-02-17 1:00 ` patchwork-bot+netdevbpf
2021-02-17 1:04 ` Florian Fainelli
2021-02-17 3:29 ` Andrew Lunn
2021-02-17 5:37 ` DENG Qingfang
2021-02-17 11:41 ` Vladimir Oltean
2021-02-17 5:40 ` DENG Qingfang
2021-02-17 11:33 ` Vladimir Oltean
2021-02-17 12:38 ` DENG Qingfang
2021-02-17 12:42 ` Vladimir Oltean
2021-02-17 14:19 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).