netdev - Re: [PATCH net-next] net: sysfs: Implement is_visible for phys_(port_id, port_name, switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250516152611.5945afae@kernel.org>
Date: Fri, 16 May 2025 15:26:11 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Yajun Deng <yajun.deng@...ux.dev>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 horms@...nel.org, andrew+netdev@...n.ch, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: sysfs: Implement is_visible for
 phys_(port_id, port_name, switch_id)

On Thu, 15 May 2025 21:02:05 +0800 Yajun Deng wrote:
> +static struct attribute *netdev_phys_attrs[] __ro_after_init = {

Why __ro_after_init and not const? I can't find the reason with 
a quick grep. This is just an array of pointers, not objects.

> +	&dev_attr_phys_port_id.attr,
> +	&dev_attr_phys_port_name.attr,
> +	&dev_attr_phys_switch_id.attr,
> +	NULL,
> +};
> +
> +static umode_t netdev_phys_is_visible(struct kobject *kobj,
> +				      struct attribute *attr, int index)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct net_device *netdev = to_net_dev(dev);
> +
> +	if (attr == &dev_attr_phys_port_id.attr) {
> +		/* The check is also done in dev_get_phys_port_id; this helps returning
> +		 * early without hitting the locking section below.
> +		 */
> +		if (!netdev->netdev_ops->ndo_get_phys_port_id)
> +			return 0;
> +	} else if (attr == &dev_attr_phys_port_name.attr) {
> +		/* The checks are also done in dev_get_phys_port_name; this helps
> +		 * returning early without hitting the locking section below.
> +		 */
> +		if (!netdev->netdev_ops->ndo_get_phys_port_name &&
> +		    !netdev->devlink_port)
> +			return 0;
> +	} else if (attr == &dev_attr_phys_switch_id.attr) {
> +		/* The checks are also done in dev_get_phys_port_name; this helps
> +		 * returning early without hitting the locking section below. This works
> +		 * because recurse is false when calling dev_get_port_parent_id.
> +		 */
> +		if (!netdev->netdev_ops->ndo_get_port_parent_id &&
> +		    !netdev->devlink_port)
> +			return 0;

I'm slightly worried some user space depends on the files existing,
but maybe ENOENT vs EOPNOTSUPP doesn't make a big difference.

Can you remove the comments, tho? I don't think they add much value.
-- 
pw-bot: cr