Received-SPF: pass (google.com: domain of linux-kernel+bounces-160530-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 26 Apr 2024 10:59:06 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Lukas Wunner <lukas@wunner.de>, Dan Williams <dan.j.williams@intel.com>
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: <662beb6ad280f_db82d29458@dwillia2-xfh.jf.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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZiYrzzk9Me1aksmE@wunner.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?bOa9NpvvXnX+jrclANyH9VkAwze9JdpP/rkd9e3SaHclmOVvhLGhrY0J9jdo?=
 =?us-ascii?Q?UdToF3o83YF2RpTm6EY4r395Y8uy+gFA7bVphB8dZ05sX5MG2FYP1EnJgiDg?=
 =?us-ascii?Q?WclYYh9B/TExqcMyhDoB3RKDfPSrwkVIiTtB0UszLq7mRmqV0K4uGJoqRV+K?=
 =?us-ascii?Q?szOHUW4eiF9DouVFA70FrAReApqtqN3wTtJmBBnzi088kzJE0SBZvYhREfeA?=
 =?us-ascii?Q?LRDhQNbx7EEQYMYNxBSfnjeysUGMUcpTlumMBkTxhezpCMfSUAhz7GSlJUZa?=
 =?us-ascii?Q?/ISn3oZYsS4HgRfdVUjzsP7RM2gX3CHkmo19UE07a63rUmb8Bn+1pajViFZD?=
 =?us-ascii?Q?FJtTBg/tWowIbdGZUs14iDkm79T2HpQwUjkVIWywOWEcxyHkCvKhN/IezcGR?=
 =?us-ascii?Q?FzsqmhXfcHFOMw8MFJF1V3NxFnV+xJ9IU+g82Rwm+8CkySDGAWRafbwZcS2W?=
 =?us-ascii?Q?Fst3PNZrGI7bG3YOIitZ7KUgGLTApE0m5o1AVpB0bbVF8V3DO4AEwLYWa2JD?=
 =?us-ascii?Q?cqQdpkJJcN7Kdv9NcL7AYRTsaoy8rJZGKP/+X0+xBFCRDM6xEgp9NdxwxzBU?=
 =?us-ascii?Q?P723nHyNOhJaZGmFpoijTQo+w6XU77pdzTMdOXZ7iBhmhgWQECdgSVQXKKsL?=
 =?us-ascii?Q?4a0bXpHR1q9haLQ6EI731Ma6AAxvx3RjgSKri/UkyP+mzj3Ct8agVoTCvl3K?=
 =?us-ascii?Q?y/Srvb2I794PHGB91+TYyU7Y6NFKS1QRc2y4eFDkM+/CNZb2wNwOlYY82Pke?=
 =?us-ascii?Q?gLTO5F0UON4w4nLTlw2mLGEwW5o+KBrj40ULSdpqMWjZkY6IgjnmA58oKppk?=
 =?us-ascii?Q?c38NL2iPoJx4oGsbEHT/pX9g9afNBy4pJzVC+ognTPaaJC+JRwb7RKzZ7ShR?=
 =?us-ascii?Q?cxFtf2WetvxpP6DM8uSZvVc1ZYL+0stF9EXpv4WOXOyCXv4AbT4+H4nK8v7J?=
 =?us-ascii?Q?n4FQrPlhsJmw/hTjZLf8IJVTyCJ7EhpD4JnoYTqPbsTQoGYd53YANEN3IWyi?=
 =?us-ascii?Q?wPKWr3Wr/mdcLIp4pQZit/zIbrKy3L5GbM47T8n3JuX9w2++ah20ilowjA7A?=
 =?us-ascii?Q?8TKPNOkA5mpVJ5yRHiMiBF+YafKY9h1RkQ64t9es3ahUYlrMqtRrS+bVvcEg?=
 =?us-ascii?Q?A8MsjstjxjrdPOLvXgbVK3WlT0W+bbl54rBDzbhfeImofHZJj3I7UM3KoD8K?=
 =?us-ascii?Q?Huqrw6TDlmrHj6tZun0EqY5Oca6DuM8kspS6rWlQ37XoGqKCZeOeXIsPU/8P?=
 =?us-ascii?Q?B9NITMvW1fRihshyRdJQZng3KJ2erFqvjjuWF4s59D97lGiENSGiemSQrM8/?=
 =?us-ascii?Q?ffG/KLlm4xKDJ1oYoporL0w0dlREXUJvVdDDrosc+ZIENbmE9LC+24lvDbSI?=
 =?us-ascii?Q?rINYa0uT15YZQZZCeyrNVUm4kL/v1XFG9luWKSI/I9pr58PzA3rwViFppl4X?=
 =?us-ascii?Q?c3QVS/YkgwP7VqAETijIk8Vl7Dclqli9o3M6kT5hUQck+BarYeBwiGdX0bnv?=
 =?us-ascii?Q?6pWJmXbsL9AdDo6eTlOu/h5w4thTPu5g5Il5ryVBpZi1+MdqtBhbbt1d1ejC?=
 =?us-ascii?Q?GyvFkyuvDLT8bAXVda4rZxxPM6a0Kpwdu09NOMMGeLGeZQBbTklBW1C+NfYD?=
 =?us-ascii?Q?Fw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f6fb6c2-0c69-4666-d5f7-08dc661a9273
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 17:59:09.4805
 (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: XRmiOD2rda2/k9TZvvr2hpmxr3i8UjvInBzCOKtIF82glPdeNwty8qn7J7Bl+VpJzXHGCsHbm15DP1+34Mt9yOD7Dx1JcZV+6SRJJcIX+xA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6901
X-OriginatorOrg: intel.com

Lukas Wunner wrote:
[..]
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -33,10 +33,10 @@ 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->is_visible)
> > +	if (grp->attrs && grp->attrs[0] && grp->is_visible)
> >  		return grp->is_visible(kobj, grp->attrs[0], 0);
> >  
> > -	if (grp->bin_attrs && grp->is_bin_visible)
> > +	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> >  		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> >  
> >  	return 0;
> 
> I'm wondering why 0 is returned by default and not SYSFS_GROUP_INVISIBLE.
> 
> An empty attribute list (containing just the NULL sentinel) will now
> result in the attribute group being visible as an empty directory.
> 
> I thought the whole point was to hide such empty directories.
> 
> Was it a conscious decision to return 0?

Perhaps there should be a comment here because yes, this is on purpose.

> Did you expect breakage if SYSFS_GROUP_INVISIBLE is returned?

Yes, the history is here:

    https://lore.kernel.org/all/YwZCPdPl2T+ndzjU@kroah.com/

..where an initial attempt to hide empty group directories resulted in
boot failures. The concern is that there might be user tooling that
depends on that empty directory. So the SYSFS_GROUP_INVISIBLE behavior
can only be enabled by explicit result from an is_visible() handler.

That way there is no regression potential for legacy cases where the
empty directory might matter.