Re: [PATCH net v5] net: systemport: Add error pointer checks in bcm_sysport_map_queues() and bcm_sysport_unmap_queues()
From: Vladimir Oltean
Date: Fri Sep 27 2024 - 08:01:42 EST
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?!