[PATCH net 0/3] Fix a bug and avoid dangerous usage patterns

[PATCH net 0/3] Fix a bug and avoid dangerous usage patterns

[Vladimir Oltean]

Making DSA use the sk_buff control block was my idea during the
'Traffic-support-for-SJA1105-DSA-driver' patchset, and I had also
introduced a series of macro helpers that turned out to not be so
helpful:

1. DSA_SKB_ZERO() zeroizes the 48-byte skb->cb area, but due to the high
   performance impact in the hotpath it was only intended to be called
   from the timestamping path. But it turns out that not zeroizing it
   has uncovered the reading of an uninitialized member field of
   DSA_SKB_CB, so in the future just be careful about what needs
   initialization and remove this macro.
2. DSA_SKB_CLONE() contains a flaw in its body definition (originally
   put there to silence checkpatch.pl) and is unusable at this point
   (will only cause NPE's when used). So remove it.
3. For DSA_SKB_COPY() the same performance considerations apply as above
   and therefore it's best to prune this function before it reaches a
   stable kernel and potentially any users.

Vladimir Oltean (3):
  net: dsa: Initialize DSA_SKB_CB(skb)->deferred_xmit variable
  net: dsa: Remove dangerous DSA_SKB_CLONE() macro
  net: dsa: Remove the now unused DSA_SKB_CB_COPY() macro

 include/net/dsa.h | 15 ---------------
 net/dsa/slave.c   |  2 ++
 2 files changed, 2 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH net 1/3] net: dsa: Initialize DSA_SKB_CB(skb)->deferred_xmit variable

[Vladimir Oltean]

The sk_buff control block can have any contents on xmit put there by the
stack, so initialization is mandatory, since we are checking its value
after the actual DSA xmit (the tagger may have changed it).

The DSA_SKB_ZERO() macro could have been used for this purpose, but:
- Zeroizing a 48-byte memory region in the hotpath is best avoided.
- It would have triggered a warning with newer compilers since
  __dsa_skb_cb contains a structure within a structure, and the {0}
  initializer was incorrect for that purpose.

So simply remove the DSA_SKB_ZERO() macro and initialize the
deferred_xmit variable by hand (which should be done for all further
dsa_skb_cb variables which need initialization - currently none - to
avoid the performance penalty).

Fixes: 97a69a0dea9a ("net: dsa: Add support for deferred xmit")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/net/dsa.h | 3 ---
 net/dsa/slave.c   | 2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 6aaaadd6a413..35ca1f2c6e28 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -102,9 +102,6 @@ struct __dsa_skb_cb {
 #define DSA_SKB_CB_COPY(nskb, skb)		\
 	{ *__DSA_SKB_CB(nskb) = *__DSA_SKB_CB(skb); }
 
-#define DSA_SKB_CB_ZERO(skb)			\
-	{ *__DSA_SKB_CB(skb) = (struct __dsa_skb_cb) {0}; }
-
 #define DSA_SKB_CB_PRIV(skb)			\
 	((void *)(skb)->cb + offsetof(struct __dsa_skb_cb, priv))
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index fe7b6a62e8f1..9892ca1f6859 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -463,6 +463,8 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	s->tx_bytes += skb->len;
 	u64_stats_update_end(&s->syncp);
 
+	DSA_SKB_CB(skb)->deferred_xmit = false;
+
 	/* Identify PTP protocol packets, clone them, and pass them to the
 	 * switch driver
 	 */
-- 
2.17.1

[PATCH net 2/3] net: dsa: Remove dangerous DSA_SKB_CLONE() macro

[Vladimir Oltean]

This does not cause any bug now because it has no users, but its body
contains two pointer definitions within a code block:

		struct sk_buff *clone = _clone;	\
		struct sk_buff *skb = _skb;	\

When calling the macro as DSA_SKB_CLONE(clone, skb), these variables
would obscure the arguments that the macro was called with, and the
initializers would be a no-op instead of doing their job (undefined
behavior, by the way, but GCC nicely puts NULL pointers instead).

So simply remove this broken macro and leave users to simply call
"DSA_SKB_CB(skb)->clone = clone" by hand when needed.

There is one functional difference when doing what I just suggested
above: the control block won't be transferred from the original skb into
the clone. Since there's no foreseen need for the control block in the
clone ATM, this is ok.

Fixes: b68b0dd0fb2d ("net: dsa: Keep private info in the skb->cb")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/net/dsa.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 35ca1f2c6e28..1f6b8608b0b7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -105,15 +105,6 @@ struct __dsa_skb_cb {
 #define DSA_SKB_CB_PRIV(skb)			\
 	((void *)(skb)->cb + offsetof(struct __dsa_skb_cb, priv))
 
-#define DSA_SKB_CB_CLONE(_clone, _skb)		\
-	{					\
-		struct sk_buff *clone = _clone;	\
-		struct sk_buff *skb = _skb;	\
-						\
-		DSA_SKB_CB_COPY(clone, skb);	\
-		DSA_SKB_CB(skb)->clone = clone; \
-	}
-
 struct dsa_switch_tree {
 	struct list_head	list;
 
-- 
2.17.1

[PATCH net 3/3] net: dsa: Remove the now unused DSA_SKB_CB_COPY() macro

[Vladimir Oltean]

It's best to not expose this, due to the performance hit it may cause
when calling it.

Fixes: b68b0dd0fb2d ("net: dsa: Keep private info in the skb->cb")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/net/dsa.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 1f6b8608b0b7..685294817712 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -99,9 +99,6 @@ struct __dsa_skb_cb {
 
 #define DSA_SKB_CB(skb) ((struct dsa_skb_cb *)((skb)->cb))
 
-#define DSA_SKB_CB_COPY(nskb, skb)		\
-	{ *__DSA_SKB_CB(nskb) = *__DSA_SKB_CB(skb); }
-
 #define DSA_SKB_CB_PRIV(skb)			\
 	((void *)(skb)->cb + offsetof(struct __dsa_skb_cb, priv))
 
-- 
2.17.1

Re: [PATCH net 0/3] Fix a bug and avoid dangerous usage patterns

[Vladimir Oltean]

On Sat, 11 May 2019 at 23:16, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Making DSA use the sk_buff control block was my idea during the
> 'Traffic-support-for-SJA1105-DSA-driver' patchset, and I had also
> introduced a series of macro helpers that turned out to not be so
> helpful:
>
> 1. DSA_SKB_ZERO() zeroizes the 48-byte skb->cb area, but due to the high
>    performance impact in the hotpath it was only intended to be called
>    from the timestamping path. But it turns out that not zeroizing it
>    has uncovered the reading of an uninitialized member field of
>    DSA_SKB_CB, so in the future just be careful about what needs
>    initialization and remove this macro.
> 2. DSA_SKB_CLONE() contains a flaw in its body definition (originally
>    put there to silence checkpatch.pl) and is unusable at this point
>    (will only cause NPE's when used). So remove it.
> 3. For DSA_SKB_COPY() the same performance considerations apply as above
>    and therefore it's best to prune this function before it reaches a
>    stable kernel and potentially any users.
>
> Vladimir Oltean (3):
>   net: dsa: Initialize DSA_SKB_CB(skb)->deferred_xmit variable
>   net: dsa: Remove dangerous DSA_SKB_CLONE() macro
>   net: dsa: Remove the now unused DSA_SKB_CB_COPY() macro
>
>  include/net/dsa.h | 15 ---------------
>  net/dsa/slave.c   |  2 ++
>  2 files changed, 2 insertions(+), 15 deletions(-)
>
> --
> 2.17.1
>

The title was "Fix a bug and avoid dangerous usage patterns [...around
DSA_SKB_CB]", not sure why it got trimmed.

Re: [PATCH net 2/3] net: dsa: Remove dangerous DSA_SKB_CLONE() macro

[Andrew Lunn]

On Sat, May 11, 2019 at 11:14:46PM +0300, Vladimir Oltean wrote:
> This does not cause any bug now because it has no users, but its body
> contains two pointer definitions within a code block:
> 
> 		struct sk_buff *clone = _clone;	\
> 		struct sk_buff *skb = _skb;	\
> 
> When calling the macro as DSA_SKB_CLONE(clone, skb), these variables
> would obscure the arguments that the macro was called with, and the
> initializers would be a no-op instead of doing their job (undefined
> behavior, by the way, but GCC nicely puts NULL pointers instead).
> 
> So simply remove this broken macro and leave users to simply call
> "DSA_SKB_CB(skb)->clone = clone" by hand when needed.
> 
> There is one functional difference when doing what I just suggested
> above: the control block won't be transferred from the original skb into
> the clone. Since there's no foreseen need for the control block in the
> clone ATM, this is ok.
> 
> Fixes: b68b0dd0fb2d ("net: dsa: Keep private info in the skb->cb")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

If it has no users, it should not of been merged.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net 3/3] net: dsa: Remove the now unused DSA_SKB_CB_COPY() macro

[Andrew Lunn]

On Sat, May 11, 2019 at 11:14:47PM +0300, Vladimir Oltean wrote:
> It's best to not expose this, due to the performance hit it may cause
> when calling it.
> 
> Fixes: b68b0dd0fb2d ("net: dsa: Keep private info in the skb->cb")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net 0/3] Fix a bug and avoid dangerous usage patterns

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Sat, 11 May 2019 23:14:44 +0300

> Making DSA use the sk_buff control block was my idea during the
> 'Traffic-support-for-SJA1105-DSA-driver' patchset, and I had also
> introduced a series of macro helpers that turned out to not be so
> helpful:
> 
> 1. DSA_SKB_ZERO() zeroizes the 48-byte skb->cb area, but due to the high
>    performance impact in the hotpath it was only intended to be called
>    from the timestamping path. But it turns out that not zeroizing it
>    has uncovered the reading of an uninitialized member field of
>    DSA_SKB_CB, so in the future just be careful about what needs
>    initialization and remove this macro.
> 2. DSA_SKB_CLONE() contains a flaw in its body definition (originally
>    put there to silence checkpatch.pl) and is unusable at this point
>    (will only cause NPE's when used). So remove it.
> 3. For DSA_SKB_COPY() the same performance considerations apply as above
>    and therefore it's best to prune this function before it reaches a
>    stable kernel and potentially any users.

Series applied, thank you.