| Message ID | 20231206071655.1626479-1-sean@geanix.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1499b89289bf272fd83cb296c82fb5519d0fe93f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: dsa: microchip: provide a list of valid protocols for xmit handler | expand |
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
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.
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
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 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>
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
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 --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,
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(-)