* [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops @ 2020-05-04 20:18 Florian Fainelli 2020-05-04 20:34 ` Vladimir Oltean ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Florian Fainelli @ 2020-05-04 20:18 UTC (permalink / raw) To: netdev Cc: allen.pais, Florian Fainelli, Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, open list When ndo_get_phys_port_name() for the CPU port was added we introduced an early check for when the DSA master network device in dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When we perform the teardown operation in dsa_master_ndo_teardown() we would not be checking that cpu_dp->orig_ndo_ops was successfully allocated and non-NULL initialized. With network device drivers such as virtio_net, this leads to a NPD as soon as the DSA switch hanging off of it gets torn down because we are now assigning the virtio_net device's netdev_ops a NULL pointer. Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") Reported-by: Allen Pais <allen.pais@oracle.com> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- net/dsa/master.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/dsa/master.c b/net/dsa/master.c index b5c535af63a3..a621367c6e8c 100644 --- a/net/dsa/master.c +++ b/net/dsa/master.c @@ -289,7 +289,8 @@ static void dsa_master_ndo_teardown(struct net_device *dev) { struct dsa_port *cpu_dp = dev->dsa_ptr; - dev->netdev_ops = cpu_dp->orig_ndo_ops; + if (cpu_dp->orig_ndo_ops) + dev->netdev_ops = cpu_dp->orig_ndo_ops; cpu_dp->orig_ndo_ops = NULL; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops 2020-05-04 20:18 [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops Florian Fainelli @ 2020-05-04 20:34 ` Vladimir Oltean 2020-05-04 20:40 ` Florian Fainelli 2020-05-05 6:07 ` Allen 2020-05-07 0:32 ` David Miller 2 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2020-05-04 20:34 UTC (permalink / raw) To: Florian Fainelli Cc: netdev, allen.pais, Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, open list Hi Florian, On Mon, 4 May 2020 at 23:19, Florian Fainelli <f.fainelli@gmail.com> wrote: > > When ndo_get_phys_port_name() for the CPU port was added we introduced > an early check for when the DSA master network device in > dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When > we perform the teardown operation in dsa_master_ndo_teardown() we would > not be checking that cpu_dp->orig_ndo_ops was successfully allocated and > non-NULL initialized. > > With network device drivers such as virtio_net, this leads to a NPD as > soon as the DSA switch hanging off of it gets torn down because we are > now assigning the virtio_net device's netdev_ops a NULL pointer. > > Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") > Reported-by: Allen Pais <allen.pais@oracle.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- The fix makes complete sense. But on another note, if we don't overlay an ndo_get_phys_port_name if the master already has one, doesn't that render the entire mechanism of having a reliable way for user space to determine the CPU port number pointless? > net/dsa/master.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/dsa/master.c b/net/dsa/master.c > index b5c535af63a3..a621367c6e8c 100644 > --- a/net/dsa/master.c > +++ b/net/dsa/master.c > @@ -289,7 +289,8 @@ static void dsa_master_ndo_teardown(struct net_device *dev) > { > struct dsa_port *cpu_dp = dev->dsa_ptr; > > - dev->netdev_ops = cpu_dp->orig_ndo_ops; > + if (cpu_dp->orig_ndo_ops) > + dev->netdev_ops = cpu_dp->orig_ndo_ops; > cpu_dp->orig_ndo_ops = NULL; > } > > -- > 2.20.1 > Regards, -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops 2020-05-04 20:34 ` Vladimir Oltean @ 2020-05-04 20:40 ` Florian Fainelli 2020-05-04 20:49 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2020-05-04 20:40 UTC (permalink / raw) To: Vladimir Oltean Cc: netdev, allen.pais, Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, open list On 5/4/2020 1:34 PM, Vladimir Oltean wrote: > Hi Florian, > > On Mon, 4 May 2020 at 23:19, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> When ndo_get_phys_port_name() for the CPU port was added we introduced >> an early check for when the DSA master network device in >> dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When >> we perform the teardown operation in dsa_master_ndo_teardown() we would >> not be checking that cpu_dp->orig_ndo_ops was successfully allocated and >> non-NULL initialized. >> >> With network device drivers such as virtio_net, this leads to a NPD as >> soon as the DSA switch hanging off of it gets torn down because we are >> now assigning the virtio_net device's netdev_ops a NULL pointer. >> >> Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") >> Reported-by: Allen Pais <allen.pais@oracle.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- > > The fix makes complete sense. > But on another note, if we don't overlay an ndo_get_phys_port_name if > the master already has one, doesn't that render the entire mechanism > of having a reliable way for user space to determine the CPU port > number pointless? For the CPU port I would consider ndo_get_phys_port_name() to be more best effort than an absolute need unlike the user facing ports, where this is necessary for a variety of actions (e.g.: determining queues/port numbers etc.) which is why there was no overlay being done in that case. There is not a good way to cascade the information other than do something like pX.Y and defining what the X and Y are, what do you think? -- Florian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops 2020-05-04 20:40 ` Florian Fainelli @ 2020-05-04 20:49 ` Vladimir Oltean 2020-05-04 21:03 ` Florian Fainelli 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2020-05-04 20:49 UTC (permalink / raw) To: Florian Fainelli Cc: netdev, allen.pais, Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, open list On Mon, 4 May 2020 at 23:40, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 5/4/2020 1:34 PM, Vladimir Oltean wrote: > > Hi Florian, > > > > On Mon, 4 May 2020 at 23:19, Florian Fainelli <f.fainelli@gmail.com> wrote: > >> > >> When ndo_get_phys_port_name() for the CPU port was added we introduced > >> an early check for when the DSA master network device in > >> dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When > >> we perform the teardown operation in dsa_master_ndo_teardown() we would > >> not be checking that cpu_dp->orig_ndo_ops was successfully allocated and > >> non-NULL initialized. > >> > >> With network device drivers such as virtio_net, this leads to a NPD as > >> soon as the DSA switch hanging off of it gets torn down because we are > >> now assigning the virtio_net device's netdev_ops a NULL pointer. > >> > >> Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") > >> Reported-by: Allen Pais <allen.pais@oracle.com> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > > > > The fix makes complete sense. > > But on another note, if we don't overlay an ndo_get_phys_port_name if > > the master already has one, doesn't that render the entire mechanism > > of having a reliable way for user space to determine the CPU port > > number pointless? > > For the CPU port I would consider ndo_get_phys_port_name() to be more > best effort than an absolute need unlike the user facing ports, where > this is necessary for a variety of actions (e.g.: determining > queues/port numbers etc.) which is why there was no overlay being done > in that case. There is not a good way to cascade the information other > than do something like pX.Y and defining what the X and Y are, what do > you think? > -- > Florian For the CPU/master port I am not actually sure who is the final consumer of the ndo_get_phys_port_name, I thought it is simply informational, with the observation that it may be unreliable in transmitting that information over. Speaking of which, if "informational" is the only purpose, could this not be used? devlink port | grep "flavour cpu" pci/0000:00:00.5/4: type notset flavour cpu port 4 spi/spi2.0/4: type notset flavour cpu port 4 spi/spi2.1/4: type notset flavour cpu port 4 Thanks, -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops 2020-05-04 20:49 ` Vladimir Oltean @ 2020-05-04 21:03 ` Florian Fainelli 0 siblings, 0 replies; 7+ messages in thread From: Florian Fainelli @ 2020-05-04 21:03 UTC (permalink / raw) To: Vladimir Oltean Cc: netdev, allen.pais, Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, open list On 5/4/2020 1:49 PM, Vladimir Oltean wrote: > On Mon, 4 May 2020 at 23:40, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> >> On 5/4/2020 1:34 PM, Vladimir Oltean wrote: >>> Hi Florian, >>> >>> On Mon, 4 May 2020 at 23:19, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> >>>> When ndo_get_phys_port_name() for the CPU port was added we introduced >>>> an early check for when the DSA master network device in >>>> dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When >>>> we perform the teardown operation in dsa_master_ndo_teardown() we would >>>> not be checking that cpu_dp->orig_ndo_ops was successfully allocated and >>>> non-NULL initialized. >>>> >>>> With network device drivers such as virtio_net, this leads to a NPD as >>>> soon as the DSA switch hanging off of it gets torn down because we are >>>> now assigning the virtio_net device's netdev_ops a NULL pointer. >>>> >>>> Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") >>>> Reported-by: Allen Pais <allen.pais@oracle.com> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> --- >>> >>> The fix makes complete sense. >>> But on another note, if we don't overlay an ndo_get_phys_port_name if >>> the master already has one, doesn't that render the entire mechanism >>> of having a reliable way for user space to determine the CPU port >>> number pointless? >> >> For the CPU port I would consider ndo_get_phys_port_name() to be more >> best effort than an absolute need unlike the user facing ports, where >> this is necessary for a variety of actions (e.g.: determining >> queues/port numbers etc.) which is why there was no overlay being done >> in that case. There is not a good way to cascade the information other >> than do something like pX.Y and defining what the X and Y are, what do >> you think? >> -- >> Florian > > For the CPU/master port I am not actually sure who is the final > consumer of the ndo_get_phys_port_name, I thought it is simply > informational, with the observation that it may be unreliable in > transmitting that information over. > Speaking of which, if "informational" is the only purpose, could this > not be used? Yes, I had not considered devlink would expose that information, ndo_phys_port_name() is there now though and since it is exposed through sysfs so reverting would be an ABI breakage. > > devlink port | grep "flavour cpu" > pci/0000:00:00.5/4: type notset flavour cpu port 4 > spi/spi2.0/4: type notset flavour cpu port 4 > spi/spi2.1/4: type notset flavour cpu port 4 > > Thanks, > -Vladimir > -- Florian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops 2020-05-04 20:18 [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops Florian Fainelli 2020-05-04 20:34 ` Vladimir Oltean @ 2020-05-05 6:07 ` Allen 2020-05-07 0:32 ` David Miller 2 siblings, 0 replies; 7+ messages in thread From: Allen @ 2020-05-05 6:07 UTC (permalink / raw) To: Florian Fainelli, netdev Cc: Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, open list > When ndo_get_phys_port_name() for the CPU port was added we introduced > an early check for when the DSA master network device in > dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When > we perform the teardown operation in dsa_master_ndo_teardown() we would > not be checking that cpu_dp->orig_ndo_ops was successfully allocated and > non-NULL initialized. > > With network device drivers such as virtio_net, this leads to a NPD as > soon as the DSA switch hanging off of it gets torn down because we are > now assigning the virtio_net device's netdev_ops a NULL pointer. > > Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") > Reported-by: Allen Pais <allen.pais@oracle.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Tested-by: Allen Pais <allen.pais@oracle.com> Thank you Florain. > --- > net/dsa/master.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/dsa/master.c b/net/dsa/master.c > index b5c535af63a3..a621367c6e8c 100644 > --- a/net/dsa/master.c > +++ b/net/dsa/master.c > @@ -289,7 +289,8 @@ static void dsa_master_ndo_teardown(struct net_device *dev) > { > struct dsa_port *cpu_dp = dev->dsa_ptr; > > - dev->netdev_ops = cpu_dp->orig_ndo_ops; > + if (cpu_dp->orig_ndo_ops) > + dev->netdev_ops = cpu_dp->orig_ndo_ops; > cpu_dp->orig_ndo_ops = NULL; > } > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops 2020-05-04 20:18 [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops Florian Fainelli 2020-05-04 20:34 ` Vladimir Oltean 2020-05-05 6:07 ` Allen @ 2020-05-07 0:32 ` David Miller 2 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2020-05-07 0:32 UTC (permalink / raw) To: f.fainelli; +Cc: netdev, allen.pais, andrew, vivien.didelot, kuba, linux-kernel From: Florian Fainelli <f.fainelli@gmail.com> Date: Mon, 4 May 2020 13:18:06 -0700 > When ndo_get_phys_port_name() for the CPU port was added we introduced > an early check for when the DSA master network device in > dsa_master_ndo_setup() already implements ndo_get_phys_port_name(). When > we perform the teardown operation in dsa_master_ndo_teardown() we would > not be checking that cpu_dp->orig_ndo_ops was successfully allocated and > non-NULL initialized. > > With network device drivers such as virtio_net, this leads to a NPD as > soon as the DSA switch hanging off of it gets torn down because we are > now assigning the virtio_net device's netdev_ops a NULL pointer. > > Fixes: da7b9e9b00d4 ("net: dsa: Add ndo_get_phys_port_name() for CPU port") > Reported-by: Allen Pais <allen.pais@oracle.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Applied and queued up for -stable, thanks Florian. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-07 0:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 20:18 [PATCH net] net: dsa: Do not leave DSA master with NULL netdev_ops Florian Fainelli 2020-05-04 20:34 ` Vladimir Oltean 2020-05-04 20:40 ` Florian Fainelli 2020-05-04 20:49 ` Vladimir Oltean 2020-05-04 21:03 ` Florian Fainelli 2020-05-05 6:07 ` Allen 2020-05-07 0:32 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).