[PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Niklas Cassel]

From: Damien Le Moal <dlemoal@kernel.org>

The SCSI Removable Media Bit (RMB) should only be set for removable media,
where the device stays and the media changes, e.g. CD-ROM or floppy.

The ATA removable media device bit is currently only defined in the CFA
(CFast) specification, to indicate that the device can have its media
removed (while the device stays).

Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
removable") introduced a change to set the RMB bit if the port has either
the eSATA bit or the hot-plug capable bit set. The reasoning was that the
author wanted his eSATA ports to get treated like a USB stick.

This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
Expectations" which has since been integrated to SPC, which states that:

"""
Reports have been received that some USB Memory Stick device servers set
the removable medium (RMB) bit to one. The rub comes when the medium is
actually removed, because... The device server is removed concurrently
with the medium removal. If there is no device server, then there is no
device server that is waiting to have removable medium inserted.

Sufficient numbers of SCSI analysts see such a device:
- not as a device that supports removable medium;
but
- as a removable, hot pluggable device.
"""

The definition of the RMB bit in the SPC specification has since been
clarified to match this.

Thus, a USB stick should not have the RMB bit set (and neither shall an
eSATA nor a hot-plug capable port).

Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
external/removable") then changed so that the RMB bit is only set for the
eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
reports of SATA devices were being automounted by udisks. However,
treating eSATA and hot-plug capable ports differently is not correct.

From the AHCI 1.3.1 spec:
Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
signal and power connectors are externally accessible via a joint signal
and power connector for blindmate device hot plug.

So a hot-plug capable port is an external port, just like commit
45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
claims.

In order to not violate the SPC specification, modify the SCSI INQUIRY
data to only set the RMB bit if the ATA device can have its media removed.

This fixes a reported problem where GNOME/udisks was automounting devices
connected to hot-plug capable ports.

Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
Tested-by: Thomas Weißschuh <linux@weissschuh.net>
Reported-by: Thomas Weißschuh <linux@weissschuh.net>
Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
[cassel: wrote commit message]
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index cdf29b178ddc..e231ad22f88a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
 		2
 	};
 
-	/* set scsi removable (RMB) bit per ata bit, or if the
-	 * AHCI port says it's external (Hotplug-capable, eSATA).
+	/*
+	 * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
+	 * device bit (which is only defined in the CFA specification) is set.
 	 */
-	if (ata_id_removable(args->id) ||
-	    (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
+	if (ata_id_removable(args->id))
 		hdr[1] |= (1 << 7);
 
 	if (args->dev->class == ATA_DEV_ZAC) {
-- 
2.45.2

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Mario Limonciello]

On 6/13/2024 12:33, Niklas Cassel wrote:
> From: Damien Le Moal <dlemoal@kernel.org>
> 
> The SCSI Removable Media Bit (RMB) should only be set for removable media,
> where the device stays and the media changes, e.g. CD-ROM or floppy.
> 
> The ATA removable media device bit is currently only defined in the CFA
> (CFast) specification, to indicate that the device can have its media
> removed (while the device stays).
> 
> Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
> removable") introduced a change to set the RMB bit if the port has either
> the eSATA bit or the hot-plug capable bit set. The reasoning was that the
> author wanted his eSATA ports to get treated like a USB stick.
> 
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
> 
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
> 
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
> 
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
> 
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
> 
> Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
> external/removable") then changed so that the RMB bit is only set for the
> eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
> reports of SATA devices were being automounted by udisks. However,
> treating eSATA and hot-plug capable ports differently is not correct.
> 
>  From the AHCI 1.3.1 spec:
> Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
> signal and power connectors are externally accessible via a joint signal
> and power connector for blindmate device hot plug.
> 
> So a hot-plug capable port is an external port, just like commit
> 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> claims.
> 
> In order to not violate the SPC specification, modify the SCSI INQUIRY
> data to only set the RMB bit if the ATA device can have its media removed.
> 
> This fixes a reported problem where GNOME/udisks was automounting devices
> connected to hot-plug capable ports.
> 
> Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
> Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [cassel: wrote commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/ata/libata-scsi.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..e231ad22f88a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
>   		2
>   	};
>   
> -	/* set scsi removable (RMB) bit per ata bit, or if the
> -	 * AHCI port says it's external (Hotplug-capable, eSATA).
> +	/*
> +	 * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> +	 * device bit (which is only defined in the CFA specification) is set.
>   	 */
> -	if (ata_id_removable(args->id) ||
> -	    (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
> +	if (ata_id_removable(args->id))
>   		hdr[1] |= (1 << 7);
>   
>   	if (args->dev->class == ATA_DEV_ZAC) {

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Thomas Weißschuh]

On 2024-06-13 19:33:53+0000, Niklas Cassel wrote:
> From: Damien Le Moal <dlemoal@kernel.org>
> 
> The SCSI Removable Media Bit (RMB) should only be set for removable media,
> where the device stays and the media changes, e.g. CD-ROM or floppy.
> 
> The ATA removable media device bit is currently only defined in the CFA
> (CFast) specification, to indicate that the device can have its media
> removed (while the device stays).
> 
> Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
> removable") introduced a change to set the RMB bit if the port has either
> the eSATA bit or the hot-plug capable bit set. The reasoning was that the
> author wanted his eSATA ports to get treated like a USB stick.
> 
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
> 
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
> 
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
> 
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
> 
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
> 
> Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
> external/removable") then changed so that the RMB bit is only set for the
> eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
> reports of SATA devices were being automounted by udisks. However,
> treating eSATA and hot-plug capable ports differently is not correct.
> 
> From the AHCI 1.3.1 spec:
> Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
> signal and power connectors are externally accessible via a joint signal
> and power connector for blindmate device hot plug.
> 
> So a hot-plug capable port is an external port, just like commit
> 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> claims.
> 
> In order to not violate the SPC specification, modify the SCSI INQUIRY
> data to only set the RMB bit if the ATA device can have its media removed.
> 
> This fixes a reported problem where GNOME/udisks was automounting devices
> connected to hot-plug capable ports.
> 
> Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
> Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [cassel: wrote commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

While I would prefer a simple revert at this point in the release cycle:

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>

This patch should also have a "Cc: stable@vger.kernel.org".

> ---
>  drivers/ata/libata-scsi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..e231ad22f88a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
>  		2
>  	};
>  
> -	/* set scsi removable (RMB) bit per ata bit, or if the
> -	 * AHCI port says it's external (Hotplug-capable, eSATA).
> +	/*
> +	 * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> +	 * device bit (which is only defined in the CFA specification) is set.
>  	 */
> -	if (ata_id_removable(args->id) ||
> -	    (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
> +	if (ata_id_removable(args->id))
>  		hdr[1] |= (1 << 7);
>  
>  	if (args->dev->class == ATA_DEV_ZAC) {
> -- 
> 2.45.2
> 

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Niklas Cassel]

On Thu, Jun 13, 2024 at 07:33:53PM +0200, Niklas Cassel wrote:
> 
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
> 
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
> 
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
> 
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
> 
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).

Since SPC-6 does make it quite clear that USB Memory Stick device servers
should not have the RMB bit set, Thomas, may I ask what udisks is using to
automount USB sticks?

Since USB sticks that follow SPC-6 clearly cannot have RMB bit set,
which means that SCSI core will set removable:
(the equivalent of:)
/sys/devices/pci0000:00/0000:00:04.0/ata3/host2/target2:0:0/2:0:0:0/block/sda/removable
to 0.


Kind regards,
Niklas

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Thomas Weißschuh]

On 2024-06-13 19:44:22+0000, Niklas Cassel wrote:
> On Thu, Jun 13, 2024 at 07:33:53PM +0200, Niklas Cassel wrote:
> > 
> > This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> > Expectations" which has since been integrated to SPC, which states that:
> > 
> > """
> > Reports have been received that some USB Memory Stick device servers set
> > the removable medium (RMB) bit to one. The rub comes when the medium is
> > actually removed, because... The device server is removed concurrently
> > with the medium removal. If there is no device server, then there is no
> > device server that is waiting to have removable medium inserted.
> > 
> > Sufficient numbers of SCSI analysts see such a device:
> > - not as a device that supports removable medium;
> > but
> > - as a removable, hot pluggable device.
> > """
> > 
> > The definition of the RMB bit in the SPC specification has since been
> > clarified to match this.
> > 
> > Thus, a USB stick should not have the RMB bit set (and neither shall an
> > eSATA nor a hot-plug capable port).
> 
> Since SPC-6 does make it quite clear that USB Memory Stick device servers
> should not have the RMB bit set, Thomas, may I ask what udisks is using to
> automount USB sticks?

As also mentioned at [0]:

/* Provide easy access to _only_ the following devices
 *
 *  - anything connected via known local buses (e.g. USB or Firewire, MMC or MemoryStick)
 *  - any device with removable media
 *
 * Be careful when extending this list as we don't want to automount
 * the world when (inadvertently) connecting to a SAN.
 */

From [1]

> Since USB sticks that follow SPC-6 clearly cannot have RMB bit set,
> which means that SCSI core will set removable:
> (the equivalent of:)
> /sys/devices/pci0000:00/0000:00:04.0/ata3/host2/target2:0:0/2:0:0:0/block/sda/removable
> to 0.

(I am not a udisks person, but we have the same problem in lsblk
regarding "RM" and "HOTPLUG" attributes)

[0] https://lore.kernel.org/all/6d5e7f17-6760-4128-a5d5-22ae2a87dadf@t-8ch.de/
[1] https://github.com/storaged-project/udisks/blob/8821a7808880ea37cdb299647c38f3a5ceb3d72a/src/udiskslinuxblock.c#L860

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Niklas Cassel]

On Thu, Jun 13, 2024 at 07:49:42PM +0200, Thomas Weißschuh wrote:
> On 2024-06-13 19:44:22+0000, Niklas Cassel wrote:
> > On Thu, Jun 13, 2024 at 07:33:53PM +0200, Niklas Cassel wrote:
> > > 
> > > This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> > > Expectations" which has since been integrated to SPC, which states that:
> > > 
> > > """
> > > Reports have been received that some USB Memory Stick device servers set
> > > the removable medium (RMB) bit to one. The rub comes when the medium is
> > > actually removed, because... The device server is removed concurrently
> > > with the medium removal. If there is no device server, then there is no
> > > device server that is waiting to have removable medium inserted.
> > > 
> > > Sufficient numbers of SCSI analysts see such a device:
> > > - not as a device that supports removable medium;
> > > but
> > > - as a removable, hot pluggable device.
> > > """
> > > 
> > > The definition of the RMB bit in the SPC specification has since been
> > > clarified to match this.
> > > 
> > > Thus, a USB stick should not have the RMB bit set (and neither shall an
> > > eSATA nor a hot-plug capable port).
> > 
> > Since SPC-6 does make it quite clear that USB Memory Stick device servers
> > should not have the RMB bit set, Thomas, may I ask what udisks is using to
> > automount USB sticks?
> 
> As also mentioned at [0]:
> 
> /* Provide easy access to _only_ the following devices
>  *
>  *  - anything connected via known local buses (e.g. USB or Firewire, MMC or MemoryStick)
>  *  - any device with removable media
>  *
>  * Be careful when extending this list as we don't want to automount
>  * the world when (inadvertently) connecting to a SAN.
>  */
> 
> From [1]
> [1] https://github.com/storaged-project/udisks/blob/8821a7808880ea37cdb299647c38f3a5ceb3d72a/src/udiskslinuxblock.c#L860

Unfortunately I can't find the definition of
udisks_drive_get_media_removable().

But looking at the name, I'm assuming that it checks if the _media_ is
removable, i.e. CD-ROM and floppy, etc., and not if the device is
removable/hot-pluggable.

So I think that the patch in $subject is the way to go.


Kind regards,
Niklas

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Sergei Shtylyov]

On 6/13/24 8:33 PM, Niklas Cassel wrote:

> From: Damien Le Moal <dlemoal@kernel.org>
> 
> The SCSI Removable Media Bit (RMB) should only be set for removable media,
> where the device stays and the media changes, e.g. CD-ROM or floppy.
> 
> The ATA removable media device bit is currently only defined in the CFA
> (CFast) specification, to indicate that the device can have its media
> removed (while the device stays).
> 
> Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
> removable") introduced a change to set the RMB bit if the port has either
> the eSATA bit or the hot-plug capable bit set. The reasoning was that the
> author wanted his eSATA ports to get treated like a USB stick.
> 
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
> 
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
> 
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
> 
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
> 
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
> 
> Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
> external/removable") then changed so that the RMB bit is only set for the
> eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
> reports of SATA devices were being automounted by udisks. However,
> treating eSATA and hot-plug capable ports differently is not correct.
> 
> From the AHCI 1.3.1 spec:
> Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
> signal and power connectors are externally accessible via a joint signal
> and power connector for blindmate device hot plug.
> 
> So a hot-plug capable port is an external port, just like commit
> 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> claims.
> 
> In order to not violate the SPC specification, modify the SCSI INQUIRY
> data to only set the RMB bit if the ATA device can have its media removed.
> 
> This fixes a reported problem where GNOME/udisks was automounting devices
> connected to hot-plug capable ports.
> 
> Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
> Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [cassel: wrote commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..e231ad22f88a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
>  		2
>  	};
>  
> -	/* set scsi removable (RMB) bit per ata bit, or if the
> -	 * AHCI port says it's external (Hotplug-capable, eSATA).
> +	/*
> +	 * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> +	 * device bit (which is only defined in the CFA specification) is set.

   It used to be defined since ATA-1; I think it was only obsoleted by ATA-8 ACS...

[...]

MBR, Sergey

Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices

[Niklas Cassel]

On Thu, Jun 13, 2024 at 10:00:30PM +0300, Sergei Shtylyov wrote:
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index cdf29b178ddc..e231ad22f88a 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
> >  		2
> >  	};
> >  
> > -	/* set scsi removable (RMB) bit per ata bit, or if the
> > -	 * AHCI port says it's external (Hotplug-capable, eSATA).
> > +	/*
> > +	 * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> > +	 * device bit (which is only defined in the CFA specification) is set.
> 
>    It used to be defined since ATA-1; I think it was only obsoleted by ATA-8 ACS...

Looking at the t13 achives, it was obsoleted by:
e06116r0: Obsolete removable media related feature sets
in 2006, ATA-8 ACS (i.e. ACS-1).

I will update the comment accordingly and send out a v2.


Kind regards,
Niklas