[PATCH] drivers: base: Remove statistics group if encryption group not created

[PATCH] drivers: base: Remove statistics group if encryption group not created

[Ewan D. Milne]

If transport_add_class_device() gets an error from sysfs_create_group() when
creating the encryption group, it does not remove the statistics group in
the error path.  Adjust the error path to do this properly.

Fixes: bd2bc528691e ("scsi: scsi_transport_fc: Introduce encryption group")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/base/transport_class.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/transport_class.c b/drivers/base/transport_class.c
index 416e9f819df5..2787967e1063 100644
--- a/drivers/base/transport_class.c
+++ b/drivers/base/transport_class.c
@@ -168,11 +168,13 @@ static int transport_add_class_device(struct attribute_container *cont,
 	if (tcont->encryption) {
 		error = sysfs_create_group(&classdev->kobj, tcont->encryption);
 		if (error)
-			goto err_del;
+			goto err_del_statistics;
 	}
 
 	return 0;
 
+err_del_statistics:
+	sysfs_remove_group(&classdev->kobj, tcont->statistics);
 err_del:
 	attribute_container_class_device_del(classdev);
 err_remove:
-- 
2.52.0

Re: [PATCH] drivers: base: Remove statistics group if encryption group not created

[Justin Tee]

Hi Ewan,

Thanks for bringing this to attention.  So, I have two comments:

1.) In the err_del_statistics label, should we check for if 
(tcont->statistics) and only then sysfs_remove_group(&classdev->kobj, 
tcont->statistics) could be called?

err_del_statistics:
         if (tcont->statistics)
                 sysfs_remove_group(&classdev->kobj, tcont->statistics);

2.) In general, if the tcont->encryption sysfs group couldn’t be 
created, why would we remove the tcont->statistics sysfs group? The 
tcont->statistics and tcont->encryption are independent of each other.  
Just because the tcont->encryption sysfs group couldn’t be created 
shouldn’t mean that we need to remove the tcont->statistics sysfs group 
too.  And, clean up of the tcont->statistics sysfs group is not lost.  
When transport_remove_classdev is called, the tcont->statistics sysfs 
group is cleaned up at that time.

Regards,
Justin

Re: [PATCH] drivers: base: Remove statistics group if encryption group not created

[Ewan Milne]

Hi Justin-

Thanks for looking at this.  I don't think I explained this well
enough.  See below.

On Mon, Jun 8, 2026 at 5:12 PM Justin Tee <justintee8345@gmail.com> wrote:
>
> Hi Ewan,
>
> Thanks for bringing this to attention.  So, I have two comments:
>
> 1.) In the err_del_statistics label, should we check for if
> (tcont->statistics) and only then sysfs_remove_group(&classdev->kobj,
> tcont->statistics) could be called?
>
> err_del_statistics:
>          if (tcont->statistics)
>                  sysfs_remove_group(&classdev->kobj, tcont->statistics);
>

I think the patch is correct as-is, because the only way to get to the
err_del_statistics: label
is from an explicit goto if an error is returned when attempting to
create the encryption group.
In order to get there the creation of the statistics group had to have
been successful first.

> 2.) In general, if the tcont->encryption sysfs group couldn’t be
> created, why would we remove the tcont->statistics sysfs group? The
> tcont->statistics and tcont->encryption are independent of each other.
> Just because the tcont->encryption sysfs group couldn’t be created
> shouldn’t mean that we need to remove the tcont->statistics sysfs group
> too.  And, clean up of the tcont->statistics sysfs group is not lost.
> When transport_remove_classdev is called, the tcont->statistics sysfs
> group is cleaned up at that time.

If the encryption group could not be created, transport_add_class_device()
would return an error, and the function would have called both
attribute_container_class_device_del() and the tclass->remove() functions.

The basic issue is that if there is an error creating the encryption group,
we clean everything up except we do not clean up the statistics group
that was created and is effectively orphaned.

Or so I think.  If the intention was to allow the creation of the encryption
group to be optional, then the code could be changed to not use a goto
to return an error.  But I am not sure what other effects this might have.

-Ewan

>
> Regards,
> Justin
>

Re: [PATCH] drivers: base: Remove statistics group if encryption group not created

[Justin Tee]

> I think the patch is correct as-is, because the only way to get to the
> err_del_statistics: label
> is from an explicit goto if an error is returned when attempting to
> create the encryption group.
> In order to get there the creation of the statistics group had to have
> been successful first.

In general, a transport_container does not have to implement an
encryption attribute_group.  Also, not every transport_container
implements a statistics attribute_group either.

Only for scsi_transport_fc’s rport_attr_cont transport_container,
because it uses both the encryption and statistics attribute_group,
that the creation of the statistics group had to have been successful
first in order to have reached err_del_statistics.  However, there are
transport_containers that don’t use statistics, for example
target_attrs and vport_attr_cont.  Although not in existence, there’s
nothing stopping a theoretical transport_container that implements
encryption and not the statistics.

> If the encryption group could not be created, transport_add_class_device()
> would return an error, and the function would have called both
> attribute_container_class_device_del() and the tclass->remove() functions.
>
> The basic issue is that if there is an error creating the encryption group,
> we clean everything up except we do not clean up the statistics group
> that was created and is effectively orphaned.
>
> Or so I think.  If the intention was to allow the creation of the encryption
> group to be optional, then the code could be changed to not use a goto
> to return an error.  But I am not sure what other effects this might have.

scsi_transport_fc doesn’t check the return value of
transport_add_device().  Nevertheless, when LLDDs call
fc_remote_port_delete() or fc_remove_host(), fc_rport_final_delete()
will eventually call transport_remove_device(), which does the
sysfs_remove_group() on the statistics group.  The clean up happens
eventually, just not immediately after a sysfs_create_group() error.

But point taken, if there was an error with creating a requested sysfs
group in transport_add_device(), then we should clean up everything
the transport_container asked for and not have it linger until
transport_remove_device().

Therefore, I’m okay with the err_del_statistics label, but I still
think we should qualify it with if (tcont->statistics).