Hi Vladimir,

On Fri, 27 Sept 2024 at 17:45, Vladimir Oltean [off-list ref] wrote:

On Fri, Sep 27, 2024 at 02:29:58PM +0300, Vladimir Oltean wrote:

quoted

quoted

quoted

+ 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 [off-list ref] (supporter:BROADCOM
SYSTEMPORT ETHERNET DRIVER)
Broadcom internal kernel review list
[off-list ref] (reviewer:BROADCOM SYSTEMPORT
ETHERNET DRIVER)
"David S. Miller" [off-list ref] (maintainer:NETWORKING DRIVERS)
Eric Dumazet [off-list ref] (maintainer:NETWORKING DRIVERS)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING DRIVERS)
Paolo Abeni [off-list ref] (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