Received-SPF: pass (google.com: domain of linux-kernel+bounces-161200-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 15:39:22 -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: <662d7e9a6d437_b6e029440@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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <662d6f24528d7_b6e0294d2@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?rxc5+Ja7aYtRZbOnnAlxG/qr/z7rxKNeLv7YkjmIQJ6wH64NZ4gCRuV3GTvv?=
 =?us-ascii?Q?2WeNCuH03ceExiTtzTNj1TFOB+x6F+qVusIx/C1v3Defz/Mmp+g/ULRAXcj8?=
 =?us-ascii?Q?LiDUr5xLRu2Cgozk8xJUClEQx7FlE+fm+pNfHG2tsX7YR489/bjxraQiy32I?=
 =?us-ascii?Q?oF4vB0cmwmmFhbYzi71Vi/3Y55VHtZC8IiTN8wdjBSPUkdWqYdNMNKd6ql2f?=
 =?us-ascii?Q?R4cVi4tiS/C4DhO1Qj/hxaan2dJeZp8fZdWVhITSH8SRcnuDqmWajXKY44qA?=
 =?us-ascii?Q?kLLW8Lstr+Eh4SdpSBsENbFZSpOSj9y9IZGQvYYBn160qufq7MWfMd67hMRf?=
 =?us-ascii?Q?LJs/6UtFrpTBIxkXI8n8mFIwOAiZrI3oT7B7X4RgGNTeJE4JQbOoRPGTP6OS?=
 =?us-ascii?Q?3IAzo2IeXhj5fH0EvA+By+dugwSiQl+mf1iBQqOr0LeDSdwKiHzq3tRodret?=
 =?us-ascii?Q?bnBiDtrr2lKgURaacDmQncdBaRhR/4gcsjKD+VCF8l4Th6YDfDPivDU455wO?=
 =?us-ascii?Q?3eQxPSZLOCh8334++49G8/RvW3UeUu7c+iJcX0r21rWjs1YtqsRVpMs2swWi?=
 =?us-ascii?Q?aCOTmJkL+OGRembyDpuMBmJpRRVqf/vS1MYwzXqrInhFWcMADGJ20RqkCMzr?=
 =?us-ascii?Q?v3neMJPeNBAzZYL0jENVp4l1LNpwDlDLiGnQpguenqX8CFwKBnWM4UH74uzE?=
 =?us-ascii?Q?aOw6Deq6/uC3jPrNsOgD9pDMHxihyNjF+AK5BwOFdk18V3T2rCEfaFvGwZAM?=
 =?us-ascii?Q?tWz8BRRf8fhZSnfLM4tv6xbB1rvmWlN0Lzm+p3SfrUjg4wiW2rjHkgE1MRIe?=
 =?us-ascii?Q?Ej2rKO7/NY2xnAg4ICiH5WQt947G05rZJwg9WV8LHz0uF1tf4oYPKT10RVFe?=
 =?us-ascii?Q?8VdpFvqv1BpxRb9f+y0Ppn/d2fKNxrXTyftb/GgRPUX5qg0kmTHATP/VLMzH?=
 =?us-ascii?Q?WJpiC0cJcidWxEhg8084IVUXFluSIbZ7nDRcDAVQ/WzZA3Ao9ft0fQG0cas/?=
 =?us-ascii?Q?MjK8AQwblj72/GO1Rl8pqqK8FKaajngxf332drpr9oSre2foVqvWMq44WD5i?=
 =?us-ascii?Q?lto6keOJqeccDp6XGzZva74glEfxmeKHxWmPnU1a4pJ3OkJ2oL+Wq6ftIsnC?=
 =?us-ascii?Q?bXT0sI9bvPnEFc0H929FsJCshyDInGeDZIDb6aNaCLxJ8ayTEnfAZmHLcbHm?=
 =?us-ascii?Q?92M2ndRhy1sWzlqMigYeof+xVNMjyxC+pD+8U8HfDyvz+2VtSoQOGQjcSOiH?=
 =?us-ascii?Q?1JzjFN40tDCMHAzs0YcwXKlP2Fy8YnUa98YrdJsGBTYshTg6NjWpZZGQ5aYz?=
 =?us-ascii?Q?CEInJZ01l9bDDzSiXRmLLfP90atpuqbkI/ooQwKg4tGaPnhkA5thghlG7kN4?=
 =?us-ascii?Q?Gf9gbGAO0VY7iFsaGoviJ64OriCg6xrCuXVRzdlQCzE2QYtpCPumbldsa3Z2?=
 =?us-ascii?Q?nSin2T/8uIKo24upFwsz3+rccVBhL4Wf3qnpnZ7ZaR0BDnGDMCFSRCxBi2Uf?=
 =?us-ascii?Q?0qt530tQkEd6ycV7f9vaWqL/0P0LRob1Y5YfejBJk+GEEfDqHE+Dd0JtAPeQ?=
 =?us-ascii?Q?KGd9DQWIThG9OwqimaI/d3J250tqrF2uO+Tvfs1rGSuhNXse62DU+8kFVl3O?=
 =?us-ascii?Q?xg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 296c6d25-ec90-42c2-dbf2-08dc670ae3c8
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2024 22:39:24.9813
 (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: 6qbpFnzOuSbVZrZmnFmhPIAjlzTdYueqxZp9N274w76jesaRYSjTo7NFTKxHiNO6uO+g81b+FOvjLAIUv7P0DeruFP5dgMlhhPfm/ZMveWQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5019
X-OriginatorOrg: intel.com

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.