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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <3f8cd57a8c9456bf65fe7d83e48f090f2dfa2999@linux.dev>
Date: Sat, 17 May 2025 02:24:17 +0000
From: "Yajun Deng" <yajun.deng@...ux.dev>
To: "Jakub Kicinski" <kuba@...nel.org>
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)

May 17, 2025 at 6:26 AM, "Jakub Kicinski" <kuba@...nel.org> wrote:



> 
> 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.
> 

These attributes in net_class_attrs had __ro_after_init before this patch.


> > 
> > + &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.
> 
Okay.

> -- 
> 
> pw-bot: cr
>