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 <[email protected]>
---
https://lore.kernel.org/netdev/[email protected]/#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;
+ return 0;
+ default:
+ return -EPROTONOSUPPORT;
+ }
}
static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port,
--
2.42.0
Hi Sean,
> -----Original Message-----
> From: Sean Nyekjaer <[email protected]>
> Sent: Wednesday, December 6, 2023 12:47 PM
> To: Woojung Huh - C21699 <[email protected]>;
> UNGLinuxDriver <[email protected]>; Andrew Lunn
> <[email protected]>; Florian Fainelli <[email protected]>; Vladimir Oltean
> <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Arun Ramadoss - I17769
> <[email protected]>; Christian Eggers <[email protected]>
> Cc: Sean Nyekjaer <[email protected]>; [email protected]; linux-
> [email protected]
> 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
> [email protected]. 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 <[email protected]>
> ---
> https://lore.kernel.org/netdev/20231205124636.1345761-1-
> [email protected]/#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
On Wed, Dec 06, 2023 at 05:22:55PM +0000, [email protected] wrote:
> NULL check is missing here.
Don't just leave it there, also explain why.
Hi Vladimir and Madhuri,
> On 6 Dec 2023, at 18.35, Vladimir Oltean <[email protected]> wrote:
>
> On Wed, Dec 06, 2023 at 05:22:55PM +0000, [email protected] 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
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.
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 <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
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 <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Vlad,
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Wednesday, December 6, 2023 11:32 PM
> To: Sean Nyekjaer <[email protected]>
> Cc: Madhuri Sripada - I34878 <[email protected]>; Woojung
> Huh - C21699 <[email protected]>; UNGLinuxDriver
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Arun Ramadoss - I17769
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> 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
Hello:
This patch was applied to bpf/bpf.git (master)
by Jakub Kicinski <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html