initiator can write to the write-protected device in demo mode

[joeue Deng]

Using 'targetcli /backstores/block/ create name dev [readonly=true]' to set
block with "ro write-thru" stat.

however, the initiator can still write to the iscsi device.

and I also find something similar,
https://bugzilla.redhat.com/show_bug.cgi?id=1036052

Is this not supported current?


the version i am using is :

targetcli-2.1.fb37-3.el7.src.rpm
python-configshell-1.1.fb14-1.el7.src.rpm
python-rtslib-2.1.fb50-1.el7.src.rpm

[Andy Grover]

On 09/10/2015 03:53 AM, joeue Deng wrote:
>  Using 'targetcli /backstores/block/ create name dev
> [readonly=true]' to
>  set block with "ro write-thru" stat.
>
>  however, the initiator can still write to the iscsi device.
>
>  and I also find something similar,
>  https://bugzilla.redhat.com/show_bug.cgi?id=1036052
>
>  Is this not supported current?
>
>  the version i am using is :
>
>  targetcli-2.1.fb37-3.el7.src.rpm
>  python-configshell-1.1.fb14-1.el7.src.rpm
>  python-rtslib-2.1.fb50-1.el7.src.rpm 
>  targetcli-fb-devel mailing list 
[CCing target-devel for their thoughts]

This is because iblock's readonly parameter just results in the block 
device being opened without FMODE_WRITE flag set. If the underlying 
block device is actually read-only then iblock will pass them down and 
they will fail in blkdev_write_iter(), like they should.

But if the iblock device is set read-only but the underlying device is 
not then we get this situation, because opening without FMODE_WRITE does 
nothing to prevent write bios from being performed.

I see two solutions:

1) Change iblock to fail write/write_same/discard requests to devices it 
has as read-only, before they get to the backing block device

2) Change targetcli/rtslib so read-only is not settable by the user, but 
instead is based on the state of the backing device (by checking 
/sys/block/<x>/ro).

I lean towards #2. I tried #1 and it works, but the original goal (see 
44bfd0185) wasn't to add a read-only feature to lio exports, it was to 
get read-only block devices working.

Thoughts?

-- Andy

[Andy Grover]

On 09/10/2015 11:18 AM, Andy Grover wrote:
>  On 09/10/2015 03:53 AM, joeue Deng wrote:
> > Using 'targetcli /backstores/block/ create name dev [readonly=true]' to
> > set block with "ro write-thru" stat.
> >
> > however, the initiator can still write to the iscsi device.
> >
> > and I also find something similar,
> > https://bugzilla.redhat.com/show_bug.cgi?id=1036052
> >
> > Is this not supported current?
> >
> > the version i am using is :
> >
> > targetcli-2.1.fb37-3.el7.src.rpm
> > python-configshell-1.1.fb14-1.el7.src.rpm
> > python-rtslib-2.1.fb50-1.el7.src.rpm
>
> > targetcli-fb-devel mailing list
>
>  [CCing target-devel for their thoughts]
>
>  This is because iblock's readonly parameter just results in the block
>  device being opened without FMODE_WRITE flag set. If the underlying
>  block device is actually read-only then iblock will pass them down and
>  they will fail in blkdev_write_iter(), like they should.
>
>  But if the iblock device is set read-only but the underlying device is
>  not then we get this situation, because opening without FMODE_WRITE does
>  nothing to prevent write bios from being performed.
>
>  I see two solutions:
>
>  1) Change iblock to fail write/write_same/discard requests to devices it
>  has as read-only, before they get to the backing block device
>
>  2) Change targetcli/rtslib so read-only is not settable by the user, but
>  instead is based on the state of the backing device (by checking
>  /sys/block/<x>/ro).
>
>  I lean towards #2. I tried #1 and it works, but the original goal (see
>  44bfd0185) wasn't to add a read-only feature to lio exports, it was to
>  get read-only block devices working. 
And of course we also have write protect on the target side.. Maybe what 
we do is #2, and then userspace can also set write protect on tpg luns 
if backed by a read-only block device.

-- Andy

[Nicholas A. Bellinger]

On Thu, 2015-09-10 at 12:16 -0700, Andy Grover wrote:
>  On 09/10/2015 11:18 AM, Andy Grover wrote:
>  > On 09/10/2015 03:53 AM, joeue Deng wrote:
>  >> Using 'targetcli /backstores/block/ create name dev [readonly=true]' to
>  >> set block with "ro write-thru" stat.
>  >>
>  >> however, the initiator can still write to the iscsi device.
>  >>
>  >> and I also find something similar,
>  >> https://bugzilla.redhat.com/show_bug.cgi?id=1036052
>  >>
>  >> Is this not supported current?
>  >>
>  >> the version i am using is :
>  >>
>  >> targetcli-2.1.fb37-3.el7.src.rpm
>  >> python-configshell-1.1.fb14-1.el7.src.rpm
>  >> python-rtslib-2.1.fb50-1.el7.src.rpm
>  >
>  >> targetcli-fb-devel mailing list
>  >
>  > [CCing target-devel for their thoughts]
>  >
>  > This is because iblock's readonly parameter just results in the block
>  > device being opened without FMODE_WRITE flag set. If the underlying
>  > block device is actually read-only then iblock will pass them down and
>  > they will fail in blkdev_write_iter(), like they should.
>  >
>  > But if the iblock device is set read-only but the underlying device is
>  > not then we get this situation, because opening without FMODE_WRITE does
>  > nothing to prevent write bios from being performed.
>  >
>  > I see two solutions:
>  >
>  > 1) Change iblock to fail write/write_same/discard requests to devices it
>  > has as read-only, before they get to the backing block device
>  >
>  > 2) Change targetcli/rtslib so read-only is not settable by the user, but
>  > instead is based on the state of the backing device (by checking
>  > /sys/block/<x>/ro).
>  >
>  > I lean towards #2. I tried #1 and it works, but the original goal (see
>  > 44bfd0185) wasn't to add a read-only feature to lio exports, it was to
>  > get read-only block devices working.
>  
>  And of course we also have write protect on the target side.. Maybe what 
>  we do is #2, and then userspace can also set write protect on tpg luns 
>  if backed by a read-only block device. 
After thinking about this a bit more, I'd prefer the following to
propagate up a read-only bit using se_device->dev_flags, and
subsequently override the passed lun_access in core_tpg_add_lun().

WDYT..?

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 5a9982f..0f19e11 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -105,6 +105,8 @@ static int iblock_configure_device(struct se_device *dev)
 	mode = FMODE_READ|FMODE_EXCL;
 	if (!ib_dev->ibd_readonly)
 		mode |= FMODE_WRITE;
+	else
+		dev->dev_flags |= DF_READ_ONLY;
 
 	bd = blkdev_get_by_path(ib_dev->ibd_udev_path, mode, ib_dev);
 	if (IS_ERR(bd)) {
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 2d0381d..5fb9dd7 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -668,7 +668,10 @@ int core_tpg_add_lun(
 	list_add_tail(&lun->lun_dev_link, &dev->dev_sep_list);
 	spin_unlock(&dev->se_port_lock);
 
-	lun->lun_access = lun_access;
+	if (dev->dev_flags & DF_READ_ONLY)
+		lun->lun_access = TRANSPORT_LUNFLAGS_READ_ONLY;
+	else
+		lun->lun_access = lun_access;
 	if (!(dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE))
 		hlist_add_head_rcu(&lun->link, &tpg->tpg_lun_hlist);
 	mutex_unlock(&tpg->tpg_lun_mutex);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index ac9bf1c..5f48754 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -730,6 +730,7 @@ struct se_device {
 #define DF_EMULATED_VPD_UNIT_SERIAL		0x00000004
 #define DF_USING_UDEV_PATH			0x00000008
 #define DF_USING_ALIAS				0x00000010
+#define DF_READ_ONLY				0x00000020
 	/* Physical device queue depth */
 	u32			queue_depth;
 	/* Used for SPC-2 reservations enforce of ISIDs */

[Andy Grover]

On 09/15/2015 05:21 PM, Nicholas A. Bellinger wrote:
>  After thinking about this a bit more, I'd prefer the following
> to
>  propagate up a read-only bit using se_device->dev_flags, and
>  subsequently override the passed lun_access in core_tpg_add_lun().
>
>  WDYT..? 
Looks like a good solution to me!

Thanks -- Andy

>  diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
>  index 5a9982f..0f19e11 100644
>  --- a/drivers/target/target_core_iblock.c
>  +++ b/drivers/target/target_core_iblock.c
>  @@ -105,6 +105,8 @@ static int iblock_configure_device(struct se_device *dev)
>    	mode = FMODE_READ|FMODE_EXCL;
>    	if (!ib_dev->ibd_readonly)
>    		mode |= FMODE_WRITE;
>  +	else
>  +		dev->dev_flags |= DF_READ_ONLY;
>
>    	bd = blkdev_get_by_path(ib_dev->ibd_udev_path, mode, ib_dev);
>    	if (IS_ERR(bd)) {
>  diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
>  index 2d0381d..5fb9dd7 100644
>  --- a/drivers/target/target_core_tpg.c
>  +++ b/drivers/target/target_core_tpg.c
>  @@ -668,7 +668,10 @@ int core_tpg_add_lun(
>    	list_add_tail(&lun->lun_dev_link, &dev->dev_sep_list);
>    	spin_unlock(&dev->se_port_lock);
>
>  -	lun->lun_access = lun_access;
>  +	if (dev->dev_flags & DF_READ_ONLY)
>  +		lun->lun_access = TRANSPORT_LUNFLAGS_READ_ONLY;
>  +	else
>  +		lun->lun_access = lun_access;
>    	if (!(dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE))
>    		hlist_add_head_rcu(&lun->link, &tpg->tpg_lun_hlist);
>    	mutex_unlock(&tpg->tpg_lun_mutex);
>  diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
>  index ac9bf1c..5f48754 100644
>  --- a/include/target/target_core_base.h
>  +++ b/include/target/target_core_base.h
>  @@ -730,6 +730,7 @@ struct se_device {
>    #define DF_EMULATED_VPD_UNIT_SERIAL		0x00000004
>    #define DF_USING_UDEV_PATH			0x00000008
>    #define DF_USING_ALIAS				0x00000010
>  +#define DF_READ_ONLY				0x00000020
>    	/* Physical device queue depth */
>    	u32			queue_depth;
>    	/* Used for SPC-2 reservations enforce of ISIDs */
>

[Nicholas A. Bellinger]

On Tue, 2015-09-15 at 18:22 -0700, Andy Grover wrote:
>  On 09/15/2015 05:21 PM, Nicholas A. Bellinger wrote:
>  > After thinking about this a bit more, I'd prefer the following to
>  > propagate up a read-only bit using se_device->dev_flags, and
>  > subsequently override the passed lun_access in core_tpg_add_lun().
>  >
>  > WDYT..?
>  
>  Looks like a good solution to me!
>   
Just pushed out this patch with a proper commit message to
target-pending/master.

Please let me know if anything else is missing.

--nab