| Message ID | 1604587039-5646-1-git-send-email-pthombar@cadence.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: macb: fix NULL dereference due to no pcs_config method | expand |
On 05/11/2020 at 15:37, Parshuram Thombare wrote: > This patch fixes NULL pointer dereference due to NULL pcs_config > in pcs_ops. > > Fixes: e4e143e26ce8 ("net: macb: add support for high speed interface") What is this tag? In linux-next? As patch is not yet in Linus' tree, you cannot refer to it like this. > Reported-by: Nicolas Ferre <Nicolas.Ferre@microchip.com> > Link: https://lkml.org/lkml/2020/11/4/482 You might need to change this to a "lore" link: https://lore.kernel.org/netdev/2db854c7-9ffb-328a-f346-f68982723d29@microchip.com/ > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> This fix looks a bit weird to me. What about proposing a patch to Russell like the chunk that you already identified in function phylink_major_config()? > --- > drivers/net/ethernet/cadence/macb_main.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index b7bc160..130a5af 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -633,6 +633,15 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs) > /* Not supported */ > } > > +static int macb_pcs_config(struct phylink_pcs *pcs, > + unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > +{ > + return 0; > +} Russell, is the requirement for this void function intended? > + > static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { > .pcs_get_state = macb_usx_pcs_get_state, > .pcs_config = macb_usx_pcs_config, > @@ -642,6 +651,7 @@ static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { > static const struct phylink_pcs_ops macb_phylink_pcs_ops = { > .pcs_get_state = macb_pcs_get_state, > .pcs_an_restart = macb_pcs_an_restart, > + .pcs_config = macb_pcs_config, > }; > > static void macb_mac_config(struct phylink_config *config, unsigned int mode, > @@ -776,10 +786,13 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode, > > if (interface == PHY_INTERFACE_MODE_10GBASER) > bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops; > - else > + else if (interface == PHY_INTERFACE_MODE_SGMII) > bp->phylink_pcs.ops = &macb_phylink_pcs_ops; Do you confirm that all SGMII type interfaces need phylink_pcs.ops? > + else > + bp->phylink_pcs.ops = NULL; > > - phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > + if (bp->phylink_pcs.ops) > + phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > > return 0; > } Regards, Nicolas
On Thu, Nov 05, 2020 at 04:22:18PM +0100, Nicolas Ferre wrote: > On 05/11/2020 at 15:37, Parshuram Thombare wrote: > > This patch fixes NULL pointer dereference due to NULL pcs_config > > in pcs_ops. > > > > Fixes: e4e143e26ce8 ("net: macb: add support for high speed interface") > > What is this tag? In linux-next? As patch is not yet in Linus' tree, you > cannot refer to it like this. > > > Reported-by: Nicolas Ferre <Nicolas.Ferre@microchip.com> > > Link: https://lkml.org/lkml/2020/11/4/482 > > You might need to change this to a "lore" link: > https://lore.kernel.org/netdev/2db854c7-9ffb-328a-f346-f68982723d29@microchip.com/ > > > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> > > This fix looks a bit weird to me. What about proposing a patch to Russell > like the chunk that you already identified in function > phylink_major_config()? No thanks. macb is currently the only case where a stub implementation for pcs_config() is required, which only occurs because the only appropriate protocol supported there is SGMII and not 1000base-X as well. > > --- > > drivers/net/ethernet/cadence/macb_main.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > > index b7bc160..130a5af 100644 > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -633,6 +633,15 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs) > > /* Not supported */ > > } > > > > +static int macb_pcs_config(struct phylink_pcs *pcs, > > + unsigned int mode, > > + phy_interface_t interface, > > + const unsigned long *advertising, > > + bool permit_pause_to_mac) > > +{ > > + return 0; > > +} > > Russell, is the requirement for this void function intended? In response to v3 of the patch on 21st October, I said, and I quote: I think all that needs to happen is a pcs_ops for the non-10GBASE-R mode which moves macb_mac_pcs_get_state() and macb_mac_an_restart() to it, and implements a stub pcs_config(). So it should be simple to do. Obviously, my advice was not followed, I didn't spot the lack of it in v4 (sorry), and the result is the NULL pointer oops.
On 05/11/2020 at 16:48, Russell King - ARM Linux admin wrote: > On Thu, Nov 05, 2020 at 04:22:18PM +0100, Nicolas Ferre wrote: >> On 05/11/2020 at 15:37, Parshuram Thombare wrote: >>> This patch fixes NULL pointer dereference due to NULL pcs_config >>> in pcs_ops. >>> >>> Fixes: e4e143e26ce8 ("net: macb: add support for high speed interface") >> >> What is this tag? In linux-next? As patch is not yet in Linus' tree, you >> cannot refer to it like this. >> >>> Reported-by: Nicolas Ferre <Nicolas.Ferre@microchip.com> >>> Link: https://lkml.org/lkml/2020/11/4/482 >> >> You might need to change this to a "lore" link: >> https://lore.kernel.org/netdev/2db854c7-9ffb-328a-f346-f68982723d29@microchip.com/ >> >>> Signed-off-by: Parshuram Thombare <pthombar@cadence.com> >> >> This fix looks a bit weird to me. What about proposing a patch to Russell >> like the chunk that you already identified in function >> phylink_major_config()? > > No thanks. macb is currently the only case where a stub implementation > for pcs_config() is required, which only occurs because the only > appropriate protocol supported there is SGMII and not 1000base-X as > well. Got it. >>> --- >>> drivers/net/ethernet/cadence/macb_main.c | 17 +++++++++++++++-- >>> 1 file changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >>> index b7bc160..130a5af 100644 >>> --- a/drivers/net/ethernet/cadence/macb_main.c >>> +++ b/drivers/net/ethernet/cadence/macb_main.c >>> @@ -633,6 +633,15 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs) >>> /* Not supported */ >>> } >>> >>> +static int macb_pcs_config(struct phylink_pcs *pcs, >>> + unsigned int mode, >>> + phy_interface_t interface, >>> + const unsigned long *advertising, >>> + bool permit_pause_to_mac) >>> +{ >>> + return 0; >>> +} >> >> Russell, is the requirement for this void function intended? > > In response to v3 of the patch on 21st October, I said, and I quote: > > I think all that needs to happen is a pcs_ops for the non-10GBASE-R > mode which moves macb_mac_pcs_get_state() and macb_mac_an_restart() > to it, and implements a stub pcs_config(). So it should be simple > to do. > > Obviously, my advice was not followed, I didn't spot the lack of it > in v4 (sorry), and the result is the NULL pointer oops. Fair enough. So Parshuram I'll add my ack tag when you re-send your patch with little issues fixed. Thanks for your help Russell. Best regards, Nicolas
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index b7bc160..130a5af 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -633,6 +633,15 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs) /* Not supported */ } +static int macb_pcs_config(struct phylink_pcs *pcs, + unsigned int mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) +{ + return 0; +} + static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { .pcs_get_state = macb_usx_pcs_get_state, .pcs_config = macb_usx_pcs_config, @@ -642,6 +651,7 @@ static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { static const struct phylink_pcs_ops macb_phylink_pcs_ops = { .pcs_get_state = macb_pcs_get_state, .pcs_an_restart = macb_pcs_an_restart, + .pcs_config = macb_pcs_config, }; static void macb_mac_config(struct phylink_config *config, unsigned int mode, @@ -776,10 +786,13 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode, if (interface == PHY_INTERFACE_MODE_10GBASER) bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops; - else + else if (interface == PHY_INTERFACE_MODE_SGMII) bp->phylink_pcs.ops = &macb_phylink_pcs_ops; + else + bp->phylink_pcs.ops = NULL; - phylink_set_pcs(bp->phylink, &bp->phylink_pcs); + if (bp->phylink_pcs.ops) + phylink_set_pcs(bp->phylink, &bp->phylink_pcs); return 0; }
This patch fixes NULL pointer dereference due to NULL pcs_config in pcs_ops. Fixes: e4e143e26ce8 ("net: macb: add support for high speed interface") Reported-by: Nicolas Ferre <Nicolas.Ferre@microchip.com> Link: https://lkml.org/lkml/2020/11/4/482 Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/net/ethernet/cadence/macb_main.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)