diff mbox series

[v2,net] net: dsa: microchip: provide a list of valid protocols for xmit handler

Message ID 20231206071655.1626479-1-sean@geanix.com
State New
Headers
Series [v2,net] net: dsa: microchip: provide a list of valid protocols for xmit handler |

Commit Message

Sean Nyekjaer Dec. 6, 2023, 7:16 a.m. UTC 
  Provide a list of valid protocols for which the driver will provide
it's deferred xmit handler.

When using DSA_TAG_PROTO_KSZ8795 protocol, it does not provide a
"connect" method, therefor ksz_connect() is not allocating ksz_tagger_data.

This avoids the following null pointer dereference:
 ksz_connect_tag_protocol from dsa_register_switch+0x9ac/0xee0
 dsa_register_switch from ksz_switch_register+0x65c/0x828
 ksz_switch_register from ksz_spi_probe+0x11c/0x168
 ksz_spi_probe from spi_probe+0x84/0xa8
 spi_probe from really_probe+0xc8/0x2d8

Fixes: ab32f56a4100 ("net: dsa: microchip: ptp: add packet transmission timestamping")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
https://lore.kernel.org/netdev/20231205124636.1345761-1-sean@geanix.com/#R
Changes since v1:
 - Provided a list of valid protocols

 drivers/net/dsa/microchip/ksz_common.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Madhuri.Sripada@microchip.com Dec. 6, 2023, 5:22 p.m. UTC | #1 
Hi Sean,

> -----Original Message-----
> From: Sean Nyekjaer <sean@geanix.com>
> Sent: Wednesday, December 6, 2023 12:47 PM
> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; Andrew Lunn
> <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>; Vladimir Oltean
> <olteanv@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; Christian Eggers <ceggers@arri.de>
> Cc: Sean Nyekjaer <sean@geanix.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v2 net] net: dsa: microchip: provide a list of valid protocols for
> xmit handler
> 
> [Some people who received this message don't often get email from
> sean@geanix.com. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Provide a list of valid protocols for which the driver will provide it's deferred
> xmit handler.
> 
> When using DSA_TAG_PROTO_KSZ8795 protocol, it does not provide a
> "connect" method, therefor ksz_connect() is not allocating ksz_tagger_data.
> 
> This avoids the following null pointer dereference:
>  ksz_connect_tag_protocol from dsa_register_switch+0x9ac/0xee0
> dsa_register_switch from ksz_switch_register+0x65c/0x828
> ksz_switch_register from ksz_spi_probe+0x11c/0x168  ksz_spi_probe from
> spi_probe+0x84/0xa8  spi_probe from really_probe+0xc8/0x2d8
> 
> Fixes: ab32f56a4100 ("net: dsa: microchip: ptp: add packet transmission
> timestamping")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> https://lore.kernel.org/netdev/20231205124636.1345761-1-
> sean@geanix.com/#R
> Changes since v1:
>  - Provided a list of valid protocols
> 
>  drivers/net/dsa/microchip/ksz_common.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 42db7679c360..286e20f340e5 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2624,10 +2624,18 @@ static int ksz_connect_tag_protocol(struct
> dsa_switch *ds,  {
>         struct ksz_tagger_data *tagger_data;
> 
> -       tagger_data = ksz_tagger_data(ds);
> -       tagger_data->xmit_work_fn = ksz_port_deferred_xmit;
> -
> -       return 0;
> +       switch (proto) {
> +       case DSA_TAG_PROTO_KSZ8795:
> +               return 0;
> +       case DSA_TAG_PROTO_KSZ9893:
> +       case DSA_TAG_PROTO_KSZ9477:
> +       case DSA_TAG_PROTO_LAN937X:
> +               tagger_data = ksz_tagger_data(ds);
> +               tagger_data->xmit_work_fn = ksz_port_deferred_xmit;


NULL check is missing here.

> +               return 0;
> +       default:
> +               return -EPROTONOSUPPORT;
> +       }
>  }
> 
>  static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port,
> --
> 2.42.0
Vladimir Oltean Dec. 6, 2023, 5:35 p.m. UTC | #2 
On Wed, Dec 06, 2023 at 05:22:55PM +0000, Madhuri.Sripada@microchip.com wrote:
> NULL check is missing here.

Don't just leave it there, also explain why.
Sean Nyekjaer Dec. 6, 2023, 5:45 p.m. UTC | #3 
Hi Vladimir and Madhuri,

> On 6 Dec 2023, at 18.35, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> On Wed, Dec 06, 2023 at 05:22:55PM +0000, Madhuri.Sripada@microchip.com wrote:
>> NULL check is missing here.

Did here what every other driver does, that uses the connect_tag_protocol() method.
(As per Vladimir’s instructions)
Not one of them, does a NULL check.

> 
> Don't just leave it there, also explain why.

Message to me?

/Sean
Vladimir Oltean Dec. 6, 2023, 6:01 p.m. UTC | #4 
On Wed, Dec 06, 2023 at 06:45:12PM +0100, Sean Nyekjaer wrote:
> > Don't just leave it there, also explain why.
> 
> Message to me?
> 
> /Sean

No, to Madhuri (as the To: field suggests).

In the Linux kernel it's not a good practice to put defensive checks
which don't have a logical justification, because other people end up
not knowing why they're there, and when they can be removed. Checking
for the tagging protocol gives a very clear indication and traceability
of why it is being done, on the other hand.

If the ds->tagger_data is NULL for a tagging protocol for which it was
expected it shouldn't be, and the DSA core still decides to call
ds->ops->connect_tag_protocol() anyway, this is a violation of the API
contract established with all drivers that use this mechanism. Papering
over a bug in the DSA core results in silent failures, which means that
any further behavior is unpredictable. So I'd very much prefer the
system to fail fast in case of a bug in the framework, so that it can be
reported and fixed. With rigorous testing, it will fail earlier than in
the production stage.

I only said "don't leave it there, also explain why" because I really
don't appreciate review comments spreading FUD, for which I'd have to
spend 20-30 minutes to explain why leaving out the NULL pointer checking
is, in fact, safe.

Of course, I am not excluding a not-yet-found bug either, but I am
strongly encouraging Madhuri to walk through the code path and point
it to us, and strongly discouraging lazy review comments. It's not fair
for me to reply to a 5 word sentence with a wall of text. So I replied
with a phrase of comparable length to the suggestion.
Vladimir Oltean Dec. 6, 2023, 8:10 p.m. UTC | #5 
On Wed, Dec 06, 2023 at 08:16:54AM +0100, Sean Nyekjaer wrote:
> Provide a list of valid protocols for which the driver will provide
> it's deferred xmit handler.
> 
> When using DSA_TAG_PROTO_KSZ8795 protocol, it does not provide a
> "connect" method, therefor ksz_connect() is not allocating ksz_tagger_data.
> 
> This avoids the following null pointer dereference:
>  ksz_connect_tag_protocol from dsa_register_switch+0x9ac/0xee0
>  dsa_register_switch from ksz_switch_register+0x65c/0x828
>  ksz_switch_register from ksz_spi_probe+0x11c/0x168
>  ksz_spi_probe from spi_probe+0x84/0xa8
>  spi_probe from really_probe+0xc8/0x2d8
> 
> Fixes: ab32f56a4100 ("net: dsa: microchip: ptp: add packet transmission timestamping")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Florian Fainelli Dec. 6, 2023, 9:53 p.m. UTC | #6 
On 12/5/23 23:16, Sean Nyekjaer wrote:
> Provide a list of valid protocols for which the driver will provide
> it's deferred xmit handler.
> 
> When using DSA_TAG_PROTO_KSZ8795 protocol, it does not provide a
> "connect" method, therefor ksz_connect() is not allocating ksz_tagger_data.
> 
> This avoids the following null pointer dereference:
>   ksz_connect_tag_protocol from dsa_register_switch+0x9ac/0xee0
>   dsa_register_switch from ksz_switch_register+0x65c/0x828
>   ksz_switch_register from ksz_spi_probe+0x11c/0x168
>   ksz_spi_probe from spi_probe+0x84/0xa8
>   spi_probe from really_probe+0xc8/0x2d8
> 
> Fixes: ab32f56a4100 ("net: dsa: microchip: ptp: add packet transmission timestamping")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Madhuri.Sripada@microchip.com Dec. 7, 2023, 10:04 a.m. UTC | #7 
Vlad,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Wednesday, December 6, 2023 11:32 PM
> To: Sean Nyekjaer <sean@geanix.com>
> Cc: Madhuri Sripada - I34878 <Madhuri.Sripada@microchip.com>; Woojung
> Huh - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; andrew@lunn.ch; f.fainelli@gmail.com;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; ceggers@arri.de;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net] net: dsa: microchip: provide a list of valid
> protocols for xmit handler
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Wed, Dec 06, 2023 at 06:45:12PM +0100, Sean Nyekjaer wrote:
> > > Don't just leave it there, also explain why.
> >
> > Message to me?
> >
> > /Sean
> 
> No, to Madhuri (as the To: field suggests).
> 
> In the Linux kernel it's not a good practice to put defensive checks which don't
> have a logical justification, because other people end up not knowing why
> they're there, and when they can be removed. Checking for the tagging
> protocol gives a very clear indication and traceability of why it is being done,
> on the other hand.
> 
> If the ds->tagger_data is NULL for a tagging protocol for which it was expected
> it shouldn't be, and the DSA core still decides to call
> ds->ops->connect_tag_protocol() anyway, this is a violation of the API
> contract established with all drivers that use this mechanism. Papering over a
> bug in the DSA core results in silent failures, which means that any further
> behavior is unpredictable. So I'd very much prefer the system to fail fast in case
> of a bug in the framework, so that it can be reported and fixed. With rigorous
> testing, it will fail earlier than in the production stage.
> 
> I only said "don't leave it there, also explain why" because I really don't
> appreciate review comments spreading FUD, for which I'd have to spend 20-
> 30 minutes to explain why leaving out the NULL pointer checking is, in fact,
> safe.
> 
> Of course, I am not excluding a not-yet-found bug either, but I am strongly
> encouraging Madhuri to walk through the code path and point it to us, and
> strongly discouraging lazy review comments. It's not fair for me to reply to a 5
> word sentence with a wall of text. So I replied with a phrase of comparable
> length to the suggestion.

I am new in this community and reviews. And was reviewing from code point of view where NULL check is a primary requirement and a general practice.
I understand the justification and will make a note of it in my further reviews and my kernel development as well.
Thanks for your inputs.

-Madhuri
patchwork-bot+netdevbpf@kernel.org Dec. 7, 2023, 6:10 p.m. UTC | #8 
Hello:

This patch was applied to bpf/bpf.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  6 Dec 2023 08:16:54 +0100 you wrote:
> Provide a list of valid protocols for which the driver will provide
> it's deferred xmit handler.
> 
> When using DSA_TAG_PROTO_KSZ8795 protocol, it does not provide a
> "connect" method, therefor ksz_connect() is not allocating ksz_tagger_data.
> 
> This avoids the following null pointer dereference:
>  ksz_connect_tag_protocol from dsa_register_switch+0x9ac/0xee0
>  dsa_register_switch from ksz_switch_register+0x65c/0x828
>  ksz_switch_register from ksz_spi_probe+0x11c/0x168
>  ksz_spi_probe from spi_probe+0x84/0xa8
>  spi_probe from really_probe+0xc8/0x2d8
> 
> [...]

Here is the summary with links:
  - [v2,net] net: dsa: microchip: provide a list of valid protocols for xmit handler
    https://git.kernel.org/bpf/bpf/c/1499b89289bf

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 42db7679c360..286e20f340e5 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2624,10 +2624,18 @@  static int ksz_connect_tag_protocol(struct dsa_switch *ds,
 {
 	struct ksz_tagger_data *tagger_data;
 
-	tagger_data = ksz_tagger_data(ds);
-	tagger_data->xmit_work_fn = ksz_port_deferred_xmit;
-
-	return 0;
+	switch (proto) {
+	case DSA_TAG_PROTO_KSZ8795:
+		return 0;
+	case DSA_TAG_PROTO_KSZ9893:
+	case DSA_TAG_PROTO_KSZ9477:
+	case DSA_TAG_PROTO_LAN937X:
+		tagger_data = ksz_tagger_data(ds);
+		tagger_data->xmit_work_fn = ksz_port_deferred_xmit;
+		return 0;
+	default:
+		return -EPROTONOSUPPORT;
+	}
 }
 
 static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port,