Received-SPF: pass (google.com: domain of linux-kernel+bounces-161201-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sat, 27 Apr 2024 16:09:26 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, Lukas Wunner <lukas@wunner.de>
CC: <gregkh@linuxfoundation.org>, Pierre-Louis Bossart
	<pierre-louis.bossart@linux.intel.com>, Marc Herbert
	<marc.herbert@intel.com>, "Rafael J. Wysocki" <rafael@kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
	<linux-coco@lists.linux.dev>, <alsa-devel@alsa-project.org>
Subject: Re: [PATCH 1/3] sysfs: Fix crash on empty group attributes array
Message-ID: <662d85a6ac2a2_b6e029449@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <170863444851.1479840.10249410842428140526.stgit@dwillia2-xfh.jf.intel.com>
 <170863445442.1479840.1818801787239831650.stgit@dwillia2-xfh.jf.intel.com>
 <ZiYrzzk9Me1aksmE@wunner.de>
 <662beb6ad280f_db82d29458@dwillia2-xfh.jf.intel.com.notmuch>
 <Ziv9984CJeQ4muZy@wunner.de>
 <662d2ca522cc6_b6e02942d@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <Zi1qtKNwcyydP4c2@wunner.de>
 <662d6f24528d7_b6e0294d2@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <662d7e9a6d437_b6e029440@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <662d7e9a6d437_b6e029440@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?sX0P8B99IWD7lez/aiSsiFDM4C716NOI0Xm6o3PUYuGLDKKLtS77pAjBsZfq?=
 =?us-ascii?Q?a+uDcSjgtfXmnvRYn9Aqty2MLtN0wdG1GbsNI1mWMHZ8KYavkxaaHdxDagIR?=
 =?us-ascii?Q?vdirUiFondoAyyOtRjar5YjYq7BxDe5o3pAlz7PuIgLZNJwmcDa7LeEfWTl1?=
 =?us-ascii?Q?vN6IP6BKK04Qyk/EMQtHXVJ4IRy7+LDUkfjBuue6xDgoKGCi6WD7V/AH+5zT?=
 =?us-ascii?Q?LtgUQ8kkmnLyQhegnU5sCBUh546r6J1CnKALKWsvsUI0uOjuoOECWsdcq+BC?=
 =?us-ascii?Q?P/H3Ua2b6aFbVSMfrACqKiLkEfULzT7R00ud6H6/NftSArFE9oIdkOQO3th7?=
 =?us-ascii?Q?6lNam+nFroWcWYIgV15ldScA2ADhWjBnsjkOSxONiB46Kl5m6lP2TrSSgc5s?=
 =?us-ascii?Q?ryr7MxHPRoQDwV2vpSoIB1UtC0NxEiDgPdNQtWyIWvVY/dCsm3MkwDI7ICcm?=
 =?us-ascii?Q?P2VKHZvqpWRa9mjfD+xWvgi5O2ba+Z1js3WqZTSDFZwWWb8z7zHSCIY84zql?=
 =?us-ascii?Q?EzYhUyhGt+tluWfJZ+9OP7sjgXpv+wNw/noC9/BJaHyousm5PifzCF0kDJs3?=
 =?us-ascii?Q?MVUXAOsbzPIvkkrmOcRDOPmxf1hlBVSoy/HCBSXhI1DL6Zuno28vvosxJFqh?=
 =?us-ascii?Q?zSyNtOmNymFZ4DjhfhGDEsmc8jdLnylIxg+fJdBzNwtOWo8W11o09x/r8tlU?=
 =?us-ascii?Q?NJz88qMsvBex6GXkjuP4gKTsUqnhu8EWWzMIPmP+Be1fcs6kXTGprz7GGYn7?=
 =?us-ascii?Q?oqe1jVoysQ/D0zh7J/X8s+V84nBU+WwQ60m7OGJkeMFrre3jrkttGY5dJxB/?=
 =?us-ascii?Q?VyBMhS/aXCx/NW7+yz/rr/iCcxb/hjMdS5u53ugKJzlUWVpeuQqa1/bVomhC?=
 =?us-ascii?Q?MHo8hf7Qt7oyRfpJvOzpqRljdPr54WWuq2DsAyVKx02+s3yH+dmyDsu038Gm?=
 =?us-ascii?Q?ukIRhV2++ZvyO2PcXUMr4kGUd0QQpYpBusqUNyLcANkGnUupU6lOkivf/PjN?=
 =?us-ascii?Q?OZiypagD67wHVeaZeRw7SB12z815UsYbNcqrWoVaIPWNCnRyrEu2QsD/gdSp?=
 =?us-ascii?Q?lm84c7TO5IgkdLPXAXGJYZ80BW159X4nHpasCcGJ9S3Ga/K5+nHHR6Rl8oV1?=
 =?us-ascii?Q?xoDB8oZ0oJmP+Hu3fWojdIHcujtnSXPm7jZusx1VIdzbntolNKBNU0VWdYfp?=
 =?us-ascii?Q?lC0UUvLgjH5mBzV9icTDIwQMpa7b+U3vnLH/wykybApJOfglq9meGTtX3Jna?=
 =?us-ascii?Q?LDgiFvO1XoVvdVoD72jo/gIiyJMIDXNThdCM6SOxA2UXsUzMHh4uKiKoA/rw?=
 =?us-ascii?Q?Xu5AxQUh8dtvNzB1XwUnmqI+EvL/90HLOdm/Dlc7y4+TtGUEPwcZWwKE/UY/?=
 =?us-ascii?Q?gR5CL0jsuhHjJ27ypyXYSA5jkJAf+LaOiTLQFyptlUZcV2fzEf+z6NfSkRDu?=
 =?us-ascii?Q?fggg1yJ+6fm+GB8j8Btgh71yAP20QgBoyYLXrU6MWZs9bVS6q089u3aMSubG?=
 =?us-ascii?Q?39swy4g8aeGJuB+v4VkRuPE9lTD0xySu8PX8e1EpMom2YJa+GYQ0kGgxzAXK?=
 =?us-ascii?Q?LeQ7fjW6N/9VA1mpUyzWBOYgvNT7qLiie4KpS/Uasvqc/2t8SYU3/wT5j+5g?=
 =?us-ascii?Q?nA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c67fdf2-3822-402b-9b86-08dc670f174d
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2024 23:09:29.3906
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SV1kSUpQSzwqaXmV7OQVthLfXKmVjzCUR5EheLk9RF+0zYcawzVnd7IvmcbulrqBPulsISeGpVoA9f2GdlXGalSSMjUB/D+XZPtbz9xcV/M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB7131
X-OriginatorOrg: intel.com

Dan Williams wrote:
> Dan Williams wrote:
> > Lukas Wunner wrote:
> > > On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:
> > > > Lukas Wunner wrote:
> > > > > But I want to raise awareness that the inability to hide
> > > > > empty attribute groups feels awkward.
> > > > 
> > > > That is fair, it was definitely some gymnastics to only change user
> > > > visible behavior for new "invisible aware" attribute groups that opt-in
> > > > while leaving all the legacy cases alone.
> > > > 
> > > > The concern is knowing when it is ok to call an is_visible() callback
> > > > with a NULL @attr argument, or knowing when an empty array actually
> > > > means "hide the group directory".
> > > > 
> > > > We could add a sentinel value to indicate "I am an empty attribute list
> > > > *AND* I want my directory hidden by default". However, that's almost
> > > > identical to requiring a placeholder attribute in the list just to make
> > > > __first_visible() happy.
> > > > 
> > > > Other ideas?
> > > 
> > > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > > which gets passed only the struct kobject pointer?
> > > 
> > > At least for PCI device authentication, that would be sufficient.
> > > I could get from the kobject to the corresponding struct device,
> > > then determine whether the device supports authentication or not.
> > > 
> > > Because it's a new, optional callback, there should be no compatibility
> > > issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > > call for individual attributes would not be needed then, at least in my
> > > use case.
> > 
> > That's where I started with this, but decided it was overkill to
> > increase the size of that data structure globally for a small number of
> > use cases.
> 
> Perhaps could try something like this:
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index d22ad67a0f32..f4054cf08e58 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -33,11 +33,23 @@ static void remove_files(struct kernfs_node *parent,
>  
>  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
>  {
> -       if (grp->attrs && grp->attrs[0] && grp->is_visible)
> -               return grp->is_visible(kobj, grp->attrs[0], 0);
> +       if (grp->attrs && grp->is_visible) {
> +               struct attribute *attr = grp->attrs[0];
> +               struct attribute empty_attr = { 0 };
>  
> -       if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> -               return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> +               if (!attr)
> +                       attr = &empty_attr;
> +               return grp->is_visible(kobj, attr, 0);
> +       }
> +
> +       if (grp->bin_attrs && grp->is_bin_visible) {
> +               struct bin_attribute *bin_attr = grp->bin_attrs[0];
> +               struct bin_attribute empty_bin_attr = { 0 };
> +
> +               if (!bin_attr)
> +                       bin_attr = &empty_bin_attr;
> +               return grp->is_bin_visible(kobj, bin_attr, 0);
> +       }
>  
>         return 0;
>  }
> 
> ...because it is highly likely that existing is_visible() callers will
> return @attr->mode when they do not recognize the attribute. But this
> could lead to some subtle bugs if something only checks the attribute
> index value. For example:
> 
> lbr_is_visible(struct kobject *kobj, struct attribute *attr, int i)
> {
>         /* branches */
>         if (i == 0)
>                 return x86_pmu.lbr_nr ? attr->mode : 0;
> 
>         return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0;
> }
> 
> ...but in this case we get lucky because it would return attr->mode
> which is 0.

Oh, but if an is_visible() callback gets confused by empty_attr, that's
detectable:

    mode = grp->is_visible(kobj, attr, 0);
    if ((mode & ~SYSFS_GROUP_INVISIBLE) && attr == empty_attr)
            return 0;
           
..i.e. the only acceptable responses to an empty_attr check is 0 or
~SYSFS_GROUP_INVISIBLE.