Re: [PATCH] scsi scsi_transport_iscsi.c: fix misuse of %llu in scsi_transport_iscsi.c

[Joe Perches]

On Sat, 2021-10-09 at 11:02 +0800, Guo Zhi wrote:
> Pointers should be printed with %p or %px rather than
> cast to (unsigned long long) and printed with %llu.
> Change %llu to %p to print the pointer into sysfs.
][]
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
[]
> @@ -129,8 +129,8 @@ show_transport_handle(struct device *dev, struct device_attribute *attr,
>  
>
>   if (!capable(CAP_SYS_ADMIN))
>   return -EACCES;
> - return sysfs_emit(buf, "%llu\n",
> - (unsigned long long)iscsi_handle(priv->iscsi_transport));
> + return sysfs_emit(buf, "%p\n",
> + iscsi_ptr(priv->iscsi_transport));

iscsi_transport is a pointer isn't it?

so why not just

return sysfs_emit(buf, "%p\n", priv->iscsi_transport);

?

[Ulrich Windl]

>>> Joe Perches <j...@perches.com> schrieb am 09.10.2021 um 05:14 in Nachricht
<5daf69b365e23ceecee911c...@perches.com>:

Isn't the difference that %p outputs hex, while %u outputs decimal?

>
> ?
>
> --
> You received this message because you are subscribed to the Google Groups
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to open-iscsi+...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/open-iscsi/5daf69b365e23ceecee911c4d0f2f66a
> 0b9ec95c.camel%40perches.com.

[Mike Christie]

On 10/11/21 1:35 AM, Ulrich Windl wrote:
>>>> Joe Perches <j...@perches.com> schrieb am 09.10.2021 um 05:14 in Nachricht
> <5daf69b365e23ceecee911c...@perches.com>:
>> On Sat, 2021-10-09 at 11:02 +0800, Guo Zhi wrote:
>>> Pointers should be printed with %p or %px rather than
>>> cast to (unsigned long long) and printed with %llu.
>>> Change %llu to %p to print the pointer into sysfs.
>> ][]
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c
>> b/drivers/scsi/scsi_transport_iscsi.c
>> []
>>> @@ -129,8 +129,8 @@ show_transport_handle(struct device *dev, struct
>> device_attribute *attr,
>>>
>>>
>>> if (!capable(CAP_SYS_ADMIN))
>>> return -EACCES;
>>> - return sysfs_emit(buf, "%llu\n",
>>> - (unsigned long long)iscsi_handle(priv->iscsi_transport));
>>> + return sysfs_emit(buf, "%p\n",
>>> + iscsi_ptr(priv->iscsi_transport));
>>
>> iscsi_transport is a pointer isn't it?
>>
>> so why not just
>>
>> return sysfs_emit(buf, "%p\n", priv->iscsi_transport);
>
> Isn't the difference that %p outputs hex, while %u outputs decimal?
>

Yeah, I think this patch will break userspace, because it doesn't know it's
a pointer. It could be doing:

sscanf(str, "%llu", &val);

The value is just later passed back to the kernel to look up a driver in
iscsi_if_transport_lookup:

list_for_each_entry(priv, &iscsi_transports, list) {
if (tt == priv->iscsi_transport) {

so we could just replace priv->transport with an int and use an ida to assign
the value.

[Ulrich Windl]

>>> Mike Christie <michael....@oracle.com> schrieb am 11.10.2021 um 17:29 in
Nachricht <ae7a82c2-5b19-493a...@oracle.com>:

I'm not in the details, but if that value is used as an ID, shouldn't it have been something like "ID%llu" right from the start?
If so it would be rather easy to use "ID%p" instead (if the syntax of the ID is left unspecified). At least nobody would treat it as a number.

Regards,
Ulrich

>
> --
> You received this message because you are subscribed to the Google Groups
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to open-iscsi+...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/open-iscsi/ae7a82c2-5b19-493a-8d61-cdccb00c
> f46c%40oracle.com.

[Guo Zhi]

Pointers should be printed with %p or %px rather than
cast to (unsigned long long) and printed with %llu.
Change %llu to %p to print the pointer into sysfs.

Signed-off-by: Guo Zhi <qtxuni...@sjtu.edu.cn>
---
drivers/scsi/scsi_transport_iscsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 922e4c7bd88e..14050c4fdb89 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -129,8 +129,7 @@ show_transport_handle(struct device *dev, struct device_attribute *attr,

if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- return sysfs_emit(buf, "%llu\n",
- (unsigned long long)iscsi_handle(priv->iscsi_transport));

+ return sysfs_emit(buf, "%p\n", priv->iscsi_transport);
}
static DEVICE_ATTR(handle, S_IRUGO, show_transport_handle, NULL);

--
2.33.0

[Guo Zhi]

Pointers should be printed with %p or %px rather than
cast to (unsigned long long) and printed with %llu.
Change %llu to %p to print the pointer into sysfs.

Signed-off-by: Guo Zhi <qtxuni...@sjtu.edu.cn>
---

drivers/scsi/scsi_transport_iscsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 922e4c7bd88e..7d6a570ebf48 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -129,8 +129,8 @@ show_transport_handle(struct device *dev, struct device_attribute *attr,

if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- return sysfs_emit(buf, "%llu\n",
- (unsigned long long)iscsi_handle(priv->iscsi_transport));
+ return sysfs_emit(buf, "%p\n",

+ iscsi_ptr(priv->iscsi_transport));

[Guo Zhi]

I will send a V2 patch.

----- 原始邮件 -----
发件人: "Joe Perches" <j...@perches.com>
收件人: "Guo Zhi" <qtxuni...@sjtu.edu.cn>, "Lee Duncan" <ldu...@suse.com>, "Chris Leech" <cle...@redhat.com>, "James E.J. Bottomley" <je...@linux.ibm.com>, "Martin K. Petersen" <martin....@oracle.com>
抄送: open-...@googlegroups.com, linux...@vger.kernel.org, "linux-kernel" <linux-...@vger.kernel.org>
发送时间: 星期六, 2021年 10 月 09日 上午 11:14:36
主题: Re: [PATCH] scsi scsi_transport_iscsi.c: fix misuse of %llu in scsi_transport_iscsi.c

[Guo Zhi]

Taking security into consideration, We should not print kernel pointer
into sysfs.

However if this is a special pointer to lookup a driver,  It's really
tricky for me to fix it,

as I don't have a scsi device to test my code.


Guo

[Ulrich Windl]

>>> Guo Zhi <qtxuni...@sjtu.edu.cn> schrieb am 15.10.2021 um 09:36 in Nachricht
<6772c5ef-4666-e2b5...@sjtu.edu.cn>:

> However if this is a special pointer to lookup a driver, It's really
> tricky for me to fix it,
>
> as I don't have a scsi device to test my code.

Years ago when a big server had 16GB RAM ;-) I was creating some RAM disks to test some RAID modes when I didn't have enough disks available.
I think you could still create an iSCSI server exporting RAM disks (in case you don't have some spare partitions or logical volumes).
Shouldn't hat be good enough to perform some tests? Maybe it can even be done on the same machine.

Regards,
Ulrich

>
>
> Guo

>
>
> --
> You received this message because you are subscribed to the Google Groups
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to open-iscsi+...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/open-iscsi/6772c5ef-4666-e2b5-2885-797baa93
> 9b45%40sjtu.edu.cn.