| Message ID | 20240926160513.7252-1-kdipendra88@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v5] net: systemport: Add error pointer checks in bcm_sysport_map_queues() and bcm_sysport_unmap_queues() | expand |
+ Vladimir On Thu, Sep 26, 2024 at 04:05:12PM +0000, Dipendra Khadka wrote: > Add error pointer checks in bcm_sysport_map_queues() and > bcm_sysport_unmap_queues() after calling dsa_port_from_netdev(). > > Fixes: 1593cd40d785 ("net: systemport: use standard netdevice notifier to detect DSA presence") > Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> Reviewed-by: Simon Horman <horms@kernel.org> > --- > v5: > -Removed extra parentheses > v4: https://lore.kernel.org/all/20240925152927.4579-1-kdipendra88@gmail.com/ > - Removed wrong and used correct Fixes: tag > v3: https://lore.kernel.org/all/20240924185634.2358-1-kdipendra88@gmail.com/ > - Updated patch subject > - Updated patch description > - Added Fixes: tags > - Fixed typo from PRT_ERR to PTR_ERR > - Error is checked just after assignment > v2: https://lore.kernel.org/all/20240923053900.1310-1-kdipendra88@gmail.com/ > - Change the subject of the patch to net > v1: https://lore.kernel.org/all/20240922181739.50056-1-kdipendra88@gmail.com/ > drivers/net/ethernet/broadcom/bcmsysport.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c > index c9faa8540859..a7ad829f11d4 100644 > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > @@ -2331,11 +2331,15 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { > static int bcm_sysport_map_queues(struct net_device *dev, > struct net_device *slave_dev) > { > - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > struct bcm_sysport_priv *priv = netdev_priv(dev); > struct bcm_sysport_tx_ring *ring; > unsigned int num_tx_queues; > unsigned int q, qp, port; > + struct dsa_port *dp; > + > + dp = dsa_port_from_netdev(slave_dev); > + if (IS_ERR(dp)) > + return PTR_ERR(dp); > > /* We can't be setting up queue inspection for non directly attached > * switches > @@ -2386,11 +2390,15 @@ static int bcm_sysport_map_queues(struct net_device *dev, > static int bcm_sysport_unmap_queues(struct net_device *dev, > struct net_device *slave_dev) > { > - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > struct bcm_sysport_priv *priv = netdev_priv(dev); > struct bcm_sysport_tx_ring *ring; > unsigned int num_tx_queues; > unsigned int q, qp, port; > + struct dsa_port *dp; > + > + dp = dsa_port_from_netdev(slave_dev); > + if (IS_ERR(dp)) > + return PTR_ERR(dp); > > port = dp->index; > > -- > 2.43.0 >
On Fri, Sep 27, 2024 at 12:02:36PM +0100, Simon Horman wrote: > + Vladimir Thanks for adding me, Simon. > On Thu, Sep 26, 2024 at 04:05:12PM +0000, Dipendra Khadka wrote: > > Add error pointer checks in bcm_sysport_map_queues() and > > bcm_sysport_unmap_queues() after calling dsa_port_from_netdev(). > > > > Fixes: 1593cd40d785 ("net: systemport: use standard netdevice notifier to detect DSA presence") > > Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> > > Reviewed-by: Simon Horman <horms@kernel.org> > > > --- > > v5: > > -Removed extra parentheses > > v4: https://lore.kernel.org/all/20240925152927.4579-1-kdipendra88@gmail.com/ > > - Removed wrong and used correct Fixes: tag > > v3: https://lore.kernel.org/all/20240924185634.2358-1-kdipendra88@gmail.com/ > > - Updated patch subject > > - Updated patch description > > - Added Fixes: tags > > - Fixed typo from PRT_ERR to PTR_ERR > > - Error is checked just after assignment > > v2: https://lore.kernel.org/all/20240923053900.1310-1-kdipendra88@gmail.com/ > > - Change the subject of the patch to net > > v1: https://lore.kernel.org/all/20240922181739.50056-1-kdipendra88@gmail.com/ > > drivers/net/ethernet/broadcom/bcmsysport.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c > > index c9faa8540859..a7ad829f11d4 100644 > > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > > @@ -2331,11 +2331,15 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { > > static int bcm_sysport_map_queues(struct net_device *dev, > > struct net_device *slave_dev) > > { > > - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > > struct bcm_sysport_priv *priv = netdev_priv(dev); > > struct bcm_sysport_tx_ring *ring; > > unsigned int num_tx_queues; > > unsigned int q, qp, port; > > + struct dsa_port *dp; > > + > > + dp = dsa_port_from_netdev(slave_dev); > > + if (IS_ERR(dp)) > > + return PTR_ERR(dp); > > > > /* We can't be setting up queue inspection for non directly attached > > * switches > > @@ -2386,11 +2390,15 @@ static int bcm_sysport_map_queues(struct net_device *dev, > > static int bcm_sysport_unmap_queues(struct net_device *dev, > > struct net_device *slave_dev) > > { > > - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > > struct bcm_sysport_priv *priv = netdev_priv(dev); > > struct bcm_sysport_tx_ring *ring; > > unsigned int num_tx_queues; > > unsigned int q, qp, port; > > + struct dsa_port *dp; > > + > > + dp = dsa_port_from_netdev(slave_dev); > > + if (IS_ERR(dp)) > > + return PTR_ERR(dp); I don't see an explanation anywhere as for why dsa_port_from_netdev() could ever return a pointer-encoded error here? hmm? Did you follow the call path and found a problem?
On Fri, Sep 27, 2024 at 02:29:58PM +0300, Vladimir Oltean wrote: > > > + dp = dsa_port_from_netdev(slave_dev); > > > + if (IS_ERR(dp)) > > > + return PTR_ERR(dp); > > I don't see an explanation anywhere as for why dsa_port_from_netdev() > could ever return a pointer-encoded error here? hmm? Did you follow the > call path and found a problem? To make my point even clearer. As the code goes: bool dsa_user_dev_check(const struct net_device *dev) { // This dereferences "dev" without a NULL pointer check. // If the kernel did not crash, it means that "dev" is not null. return dev->netdev_ops == &dsa_user_netdev_ops; } static int bcm_sysport_netdevice_event(struct notifier_block *nb, unsigned long event, void *ptr) { ... switch (event) { case NETDEV_CHANGEUPPER: ... if (!dsa_user_dev_check(info->upper_dev)) return NOTIFY_DONE; // we know here that dsa_user_dev_check() is true, and // no one changes dev->netdev_ops at runtime, to suspect // it could become false after it just returned true. // Even if it did, we are under rtnl_lock(), and whoever // did that better also acquired rtnl_lock(). Thus, // there is enough guarantee that this also remains true // below. if (info->linking) ret = bcm_sysport_map_queues(dev, info->upper_dev); else ret = bcm_sysport_unmap_queues(dev, info->upper_dev); } ... } struct dsa_port *dsa_port_from_netdev(struct net_device *netdev) { if (!netdev || !dsa_user_dev_check(netdev)) return ERR_PTR(-ENODEV); return dsa_user_to_port(netdev); } static int bcm_sysport_map_queues(struct net_device *dev, struct net_device *slave_dev) { struct dsa_port *dp = dsa_port_from_netdev(slave_dev); ... } So, if both conditions for dsa_port_from_netdev() to return ERR_PTR(-ENODEV) can only be false, why would we add an error check? Is it to appease a static analysis tool which doesn't analyze things very far? Or is there an actual problem? And why does this have a Fixes: tag and the expectation to be included as a bug fix to stable kernels? And why is the author of the blamed patch even CCed only at v5?!
Hi Vladimir, On Fri, 27 Sept 2024 at 17:45, Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Fri, Sep 27, 2024 at 02:29:58PM +0300, Vladimir Oltean wrote: > > > > + dp = dsa_port_from_netdev(slave_dev); > > > > + if (IS_ERR(dp)) > > > > + return PTR_ERR(dp); > > > > I don't see an explanation anywhere as for why dsa_port_from_netdev() > > could ever return a pointer-encoded error here? hmm? Did you follow the > > call path and found a problem? > Yeah, you are right. I ran smatch to find this and saw there is no validation. I did not see any problem as you said. I thought it would be better to include this change. If you say there is no point for this change, then that's also fine for me. I got the chance to learn new things. > To make my point even clearer. As the code goes: > > bool dsa_user_dev_check(const struct net_device *dev) > { > // This dereferences "dev" without a NULL pointer check. > // If the kernel did not crash, it means that "dev" is not null. > return dev->netdev_ops == &dsa_user_netdev_ops; > } > > static int bcm_sysport_netdevice_event(struct notifier_block *nb, > unsigned long event, void *ptr) > { > ... > switch (event) { > case NETDEV_CHANGEUPPER: > ... > if (!dsa_user_dev_check(info->upper_dev)) > return NOTIFY_DONE; > > // we know here that dsa_user_dev_check() is true, and > // no one changes dev->netdev_ops at runtime, to suspect > // it could become false after it just returned true. > // Even if it did, we are under rtnl_lock(), and whoever > // did that better also acquired rtnl_lock(). Thus, > // there is enough guarantee that this also remains true > // below. > if (info->linking) > ret = bcm_sysport_map_queues(dev, info->upper_dev); > else > ret = bcm_sysport_unmap_queues(dev, info->upper_dev); > } > ... > } > > struct dsa_port *dsa_port_from_netdev(struct net_device *netdev) > { > if (!netdev || !dsa_user_dev_check(netdev)) > return ERR_PTR(-ENODEV); > > return dsa_user_to_port(netdev); > } > > static int bcm_sysport_map_queues(struct net_device *dev, > struct net_device *slave_dev) > { > struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > ... > } > > So, if both conditions for dsa_port_from_netdev() to return ERR_PTR(-ENODEV) > can only be false, why would we add an error check? Is it to appease a > static analysis tool which doesn't analyze things very far? Or is there > an actual problem? > > And why does this have a Fixes: tag and the expectation to be included > as a bug fix to stable kernels? > > And why is the author of the blamed patch even CCed only at v5?! Sorry to know this, I ran the script and there I did not find your name. ./scripts/get_maintainer.pl drivers/net/ethernet/broadcom/bcmsysport.c Florian Fainelli <florian.fainelli@broadcom.com> (supporter:BROADCOM SYSTEMPORT ETHERNET DRIVER) Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> (reviewer:BROADCOM SYSTEMPORT ETHERNET DRIVER) "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS) Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS) Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS) Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS) netdev@vger.kernel.org (open list:BROADCOM SYSTEMPORT ETHERNET DRIVER) linux-kernel@vger.kernel.org (open list) Thank you so much for your time and effort , special thanks to Simon for everything ,thanks Vladimir for the way you explained. and thanks Florian for your help. Best regards, Dipendra Khadka
On Mon, Sep 30, 2024 at 11:52:45PM +0545, Dipendra Khadka wrote: > > And why is the author of the blamed patch even CCed only at v5?! > > Sorry to know this, I ran the script and there I did not find your name. > > ./scripts/get_maintainer.pl drivers/net/ethernet/broadcom/bcmsysport.c > Florian Fainelli <florian.fainelli@broadcom.com> (supporter:BROADCOM SYSTEMPORT ETHERNET DRIVER) > Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> (reviewer:BROADCOM SYSTEMPORT ETHERNET DRIVER) > "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS) > Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS) > Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS) > Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS) > netdev@vger.kernel.org (open list:BROADCOM SYSTEMPORT ETHERNET DRIVER) > linux-kernel@vger.kernel.org (open list) It's in the question you ask. Am I a maintainer of bcmsysport? No, and I haven't made significant contributions on it either. But if you run get_maintainer.pl on the _patch_ file that you will run through git send-email, my name will be listed (the "--fixes" option defaults to 1). The netdev CI also runs get_maintainers.pl on the actual patch, and that triggers one of its red flags: "1 blamed authors not CCed: vladimir.oltean@nxp.com" https://patchwork.kernel.org/project/netdevbpf/patch/20240926160513.7252-1-kdipendra88@gmail.com/
Hi, On Tue, 1 Oct 2024 at 02:17, Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Mon, Sep 30, 2024 at 11:52:45PM +0545, Dipendra Khadka wrote: > > > And why is the author of the blamed patch even CCed only at v5?! > > > > Sorry to know this, I ran the script and there I did not find your name. > > > > ./scripts/get_maintainer.pl drivers/net/ethernet/broadcom/bcmsysport.c > > Florian Fainelli <florian.fainelli@broadcom.com> (supporter:BROADCOM SYSTEMPORT ETHERNET DRIVER) > > Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> (reviewer:BROADCOM SYSTEMPORT ETHERNET DRIVER) > > "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS) > > Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS) > > Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS) > > Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS) > > netdev@vger.kernel.org (open list:BROADCOM SYSTEMPORT ETHERNET DRIVER) > > linux-kernel@vger.kernel.org (open list) > > It's in the question you ask. Am I a maintainer of bcmsysport? No, and > I haven't made significant contributions on it either. But if you run > get_maintainer.pl on the _patch_ file that you will run through git > send-email, my name will be listed (the "--fixes" option defaults to 1). > Oh, thank you for this. I only used to run get_maintainers.pl on the file which got changed. I will run on the patch file as well from now. > The netdev CI also runs get_maintainers.pl on the actual patch, and that > triggers one of its red flags: "1 blamed authors not CCed: vladimir.oltean@nxp.com" > https://patchwork.kernel.org/project/netdevbpf/patch/20240926160513.7252-1-kdipendra88@gmail.com/ Best regards, Dipendra Khadka
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index c9faa8540859..a7ad829f11d4 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -2331,11 +2331,15 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { static int bcm_sysport_map_queues(struct net_device *dev, struct net_device *slave_dev) { - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); struct bcm_sysport_priv *priv = netdev_priv(dev); struct bcm_sysport_tx_ring *ring; unsigned int num_tx_queues; unsigned int q, qp, port; + struct dsa_port *dp; + + dp = dsa_port_from_netdev(slave_dev); + if (IS_ERR(dp)) + return PTR_ERR(dp); /* We can't be setting up queue inspection for non directly attached * switches @@ -2386,11 +2390,15 @@ static int bcm_sysport_map_queues(struct net_device *dev, static int bcm_sysport_unmap_queues(struct net_device *dev, struct net_device *slave_dev) { - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); struct bcm_sysport_priv *priv = netdev_priv(dev); struct bcm_sysport_tx_ring *ring; unsigned int num_tx_queues; unsigned int q, qp, port; + struct dsa_port *dp; + + dp = dsa_port_from_netdev(slave_dev); + if (IS_ERR(dp)) + return PTR_ERR(dp); port = dp->index;
Add error pointer checks in bcm_sysport_map_queues() and bcm_sysport_unmap_queues() after calling dsa_port_from_netdev(). Fixes: 1593cd40d785 ("net: systemport: use standard netdevice notifier to detect DSA presence") Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> --- v5: -Removed extra parentheses v4: https://lore.kernel.org/all/20240925152927.4579-1-kdipendra88@gmail.com/ - Removed wrong and used correct Fixes: tag v3: https://lore.kernel.org/all/20240924185634.2358-1-kdipendra88@gmail.com/ - Updated patch subject - Updated patch description - Added Fixes: tags - Fixed typo from PRT_ERR to PTR_ERR - Error is checked just after assignment v2: https://lore.kernel.org/all/20240923053900.1310-1-kdipendra88@gmail.com/ - Change the subject of the patch to net v1: https://lore.kernel.org/all/20240922181739.50056-1-kdipendra88@gmail.com/ drivers/net/ethernet/broadcom/bcmsysport.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)