Re: [SCSI] qla4xxx: support iscsiadm session mgmt
On Aug 8, 2012, at 11:57 AM, Dan Carpenter wrote: > On Wed, Aug 08, 2012 at 10:35:44AM -0500, Mike Christie wrote: >> On 08/08/2012 10:00 AM, Dan Carpenter wrote: >>> I never heard back on this. This buffer overflow is still present >>> in the current code. >>> >> >> Qlogic just sent a patch yesterday. >> http://marc.info/?l=linux-scsi&m=134434199930938&w=2 > > Ah, good. > > It seems like qla4xxx_ep_connect() should take a pointer to struct > sockaddr_storage and also dst_addr in qla4xxx_get_ep_fwdb() should > be changed as well. As in: > > static struct iscsi_endpoint * > -qla4xxx_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, > +qla4xxx_ep_connect(struct Scsi_Host *shost, struct sockaddr_storage > *dst_addr, >int non_blocking) Do you mean that should be done to fix a bug or to just make it nicer? I think it will not be a issue bug wise, because it ends up getting cast to the proper struct in the end and the proper size to copy is detected. What actually gets passed that function above is not a struct sockaddr_storage. It is a sockaddr_in or socaddr_in6. It depends on if it is ipv6 or not. If you are just saying it would be nicer to be consistent in the struct used then it is a larger change that I think should be done in another patch. It will affect other drivers.-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tcmu: Fix trailing semicolon
On 01/16/2018 09:34 AM, Luis de Bethencourt wrote: > The trailing semicolon is an empty statement that does no operation. > It is completely stripped out by the compiler. Removing it since it doesn't do > anything. > > Signed-off-by: Luis de Bethencourt > --- > > Hi, > > After fixing the same thing in drivers/staging/rtl8723bs/, Joe Perches > suggested I fix it treewide [0]. > > Best regards > Luis > > > [0] > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115410.html > [1] > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115390.html > > drivers/target/target_core_user.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_user.c > b/drivers/target/target_core_user.c > index 60c8a87b7a88..f6164d294fb3 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -1564,7 +1564,7 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev > *udev) > > wake_up_all(&udev->nl_cmd_wq); > > - return ret;; > + return ret; > } > > static int tcmu_netlink_event(struct tcmu_dev *udev, enum tcmu_genl_cmd cmd, > Acked-by: Mike Christie
Re: [PATCH] target: Fix a double put in transport_free_session
On 3/22/21 9:58 PM, Lv Yunlong wrote:
> In transport_free_session, se_nacl is got from se_sess
> with the initial reference. If se_nacl->acl_sess_list is
> empty, se_nacl->dynamic_stop is set to true. Then the first
> target_put_nacl(se_nacl) will drop the initial reference
> and free se_nacl. Later there is a second target_put_nacl()
> to put se_nacl. It may cause error in race.
>> My patch sets se_nacl->dynamic_stop to false to avoid the
> double put.
>
> Signed-off-by: Lv Yunlong
> ---
> drivers/target/target_core_transport.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index 5ecb9f18a53d..c266defe694f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -584,8 +584,10 @@ void transport_free_session(struct se_session *se_sess)
> }
> mutex_unlock(&se_tpg->acl_node_mutex);
>
> - if (se_nacl->dynamic_stop)
> + if (se_nacl->dynamic_stop) {
> target_put_nacl(se_nacl);
> + se_nacl->dynamic_stop = false;
> + }
>
> target_put_nacl(se_nacl);
Could you describe the race a little more?
Is the race:
1. thread1 called core_tpg_check_initiator_node_acl and found the acl.
sess->se_node_acl is set to the found acl.
2. thread2 is running transport_free_session. It now grabs the acl_node_mutex
and sees se_nacl->acl_sess_list is empty.
3. thread2 does the dynamic_stop=true operations in transport_free_session.
4. thread1 now calls transport_register_session now adds the sess to acl's
acl_sess_list.
Later when the session that thread 1 created is deleted dynamic_stop is still
set, so we do an extra target_put_nacl?
I'm not sure your patch will handle this race. When we delete the session
thread1
created dynamic_node_acl is still set, so this:
mutex_lock(&se_tpg->acl_node_mutex);
if (se_nacl->dynamic_node_acl &&
!se_tfo->tpg_check_demo_mode_cache(se_tpg)) {
spin_lock_irqsave(&se_nacl->nacl_sess_lock, flags);
if (list_empty(&se_nacl->acl_sess_list))
se_nacl->dynamic_stop = true;
can set dynamic_stop to true again and we can end up doing the extra put still.
On top of the extra put we also do
list_del(&se_nacl->acl_list);
twice so we have to handle that as well.
Is there also another bug in this code. If someone adds an acl while there is a
dynamic acl in place core_tpg_add_initiator_node_acl will clear dynamic_node_acl
but we leave the extra reference, so later when transport_free_session is called
we will not do the extra put.
Re: [PATCH V2] scsi: iscsi_tcp: Fix use-after-free when do get_host_param
On 3/21/21 1:47 AM, Wu Bo wrote:
> From: Wu Bo
>
> iscsid(cpu1): Logout of iscsi session, will do destroy session,
> tcp_sw_host->session is not set to NULL before release the iscsi session.
> in the iscsi_sw_tcp_session_destroy().
>
> iscsadm(cpu2): Get host parameters access to tcp_sw_host->session in the
> iscsi_sw_tcp_host_get_param(), tcp_sw_host->session is not NULL,
> but pointed to a freed space.
>
> Add ihost->lock and kref to protect the session,
> between get host parameters and destroy iscsi session.
>
> [29844.848044] sd 2:0:0:1: [sdj] Synchronizing SCSI cache
> [29844.923745] scsi 2:0:0:1: alua: Detached
> [29844.927840]
> ==
> [29844.927861] BUG: KASAN: use-after-free in
> iscsi_sw_tcp_host_get_param+0xf4/0x218 [iscsi_tcp]
> [29844.927864] Read of size 8 at addr 80002c0b8f68 by task iscsiadm/523945
> [29844.927871] CPU: 1 PID: 523945 Comm: iscsiadm Kdump: loaded Not tainted
> 4.19.90.kasan.aarch64
> [29844.927873] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> [29844.927875] Call trace:
> [29844.927884] dump_backtrace+0x0/0x270
> [29844.927886] show_stack+0x24/0x30
> [29844.927895] dump_stack+0xc4/0x120
> [29844.927902] print_address_description+0x68/0x278
> [29844.927904] kasan_report+0x20c/0x338
> [29844.927906] __asan_load8+0x88/0xb0
> [29844.927910] iscsi_sw_tcp_host_get_param+0xf4/0x218 [iscsi_tcp]
> [29844.927932] show_host_param_ISCSI_HOST_PARAM_IPADDRESS+0x84/0xa0
> [scsi_transport_iscsi]
> [29844.927938] dev_attr_show+0x48/0x90
> [29844.927943] sysfs_kf_seq_show+0x100/0x1e0
> [29844.927946] kernfs_seq_show+0x88/0xa0
> [29844.927949] seq_read+0x164/0x748
> [29844.927951] kernfs_fop_read+0x204/0x308
> [29844.927956] __vfs_read+0xd4/0x2d8
> [29844.927958] vfs_read+0xa8/0x198
> [29844.927960] ksys_read+0xd0/0x180
> [29844.927962] __arm64_sys_read+0x4c/0x60
> [29844.927966] el0_svc_common+0xa8/0x230
> [29844.927969] el0_svc_handler+0xdc/0x138
> [29844.927971] el0_svc+0x10/0x218
>
> [29844.928063] Freed by task 53358:
> [29844.928066] __kasan_slab_free+0x120/0x228
> [29844.928068] kasan_slab_free+0x10/0x18
> [29844.928069] kfree+0x98/0x278
> [29844.928083] iscsi_session_release+0x84/0xa0 [scsi_transport_iscsi]
> [29844.928085] device_release+0x4c/0x100
> [29844.928089] kobject_put+0xc4/0x288
> [29844.928091] put_device+0x24/0x30
> [29844.928105] iscsi_free_session+0x60/0x70 [scsi_transport_iscsi]
> [29844.928112] iscsi_session_teardown+0x134/0x158 [libiscsi]
> [29844.928116] iscsi_sw_tcp_session_destroy+0x7c/0xd8 [iscsi_tcp]
> [29844.928129] iscsi_if_rx+0x1538/0x1f00 [scsi_transport_iscsi]
> [29844.928131] netlink_unicast+0x338/0x3c8
> [29844.928133] netlink_sendmsg+0x51c/0x588
> [29844.928135] sock_sendmsg+0x74/0x98
> [29844.928137] ___sys_sendmsg+0x434/0x470
> [29844.928139] __sys_sendmsg+0xd4/0x148
> [29844.928141] __arm64_sys_sendmsg+0x50/0x60
> [29844.928143] el0_svc_common+0xa8/0x230
> [29844.928146] el0_svc_handler+0xdc/0x138
> [29844.928147] el0_svc+0x10/0x218
> [29844.928148]
> [29844.928150] The buggy address belongs to the object at
> 80002c0b8880#012 which belongs to the cache kmalloc-2048 of size 2048
> [29844.928153] The buggy address is located 1768 bytes inside of#012
> 2048-byte region [80002c0b8880, 80002c0b9080)
> [29844.928154] The buggy address belongs to the page:
> [29844.928158] page:7eb02e00 count:1 mapcount:0
> mapping:8000d8402600 index:0x0 compound_mapcount: 0
> [29844.928902] flags: 0x7fffe008100(slab|head)
> [29844.929215] raw: 07fffe008100 7e0003535e08 7e00024a9408
> 8000d8402600
> [29844.929217] raw: 000f000f 0001
>
> [29844.929219] page dumped because: kasan: bad access detected
> [29844.929219]
> [29844.929221] Memory state around the buggy address:
> [29844.929223] 80002c0b8e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb
> [29844.929225] 80002c0b8e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb
> [29844.929227] >80002c0b8f00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb
> [29844.929228] ^
> [29844.929230] 80002c0b8f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb
> [29844.929232] 80002c0b9000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb
> [29844.929232]
> ==
> [29844.929234] Disabling lock debugging due to kernel taint
> [29844.969534] scsi host2: iSCSI Initiator over TCP/IP
>
> Fixes: a79af8a64d39 ("[SCSI] iscsi_tcp: use iscsi_conn_get_addr_param
> libiscsi function")
> Signed-off-by: Wu Bo
> Signed-off-by: WenChao Hao
> ---
> drivers/scsi/iscsi_tcp.c | 23 +--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 93ce990..579aa80 100644
>
Re: [PATCH] iscsi: Do Not set param when sock is NULL
> On Nov 4, 2020, at 11:40 PM, Gulam Mohamed wrote:
>
> Description
> =
> 1. This Kernel panic could be due to a timing issue when there is a race
> between the sync thread and the initiator was processing of a login response
> from the target. The session re-open can be invoked from two places
> a. Sessions sync thread when the iscsid restart
> b. From iscsid through iscsi error handler
> 2. The session reopen sequence is as follows in user-space
> (iscsi-initiator-utils)
> a. Disconnect the connection
> b. Then send the stop connection request to the kernel which
> releases the connection (releases the socket)
> c. Queues the reopen for 2 seconds delay
> d.Once the delay expires, create the TCP connection again by
> calling the connect() call
> e.Poll for the connection
> f. When poll is successful i.e when the TCP connection is
> established, it performs
> i. Creation of session and connection data structures
> ii. Bind the connection to the session. This is the place where we
> assign the sock to tcp_sw_conn->sock
> iii. Sets the parameters like target name, persistent address etc .
> iv. Creates the login pdu
> v. Sends the login pdu to kernel
> vi. Returns to the main loop to process further events. The kernel then
> sends the login request over to the target node
> g. Once login response with success is received, it enters into full
> feature phase and sets the negotiable parameters like max_recv_data_length,
> max_transmit_length, data_digest etc .
> 3. While setting the negotiable parameters by calling
> "iscsi_session_set_neg_params()", kernel panicked as sock was NULL
>
> What happened here is
>
> 1.Before initiator received the login response mentioned in above point
> 2.f.v, another reopen request was sent from the error handler/sync session
> for the same session, as the initiator utils was in main loop to process
> further events (as
> mentioned in point 2.f.vi above).
> 2.While processing this reopen, it stopped the connection which released
> the socket and queued this connection and at this point of time the login
> response was received for the earlier one
To make sure I got this you are saying before iscsi_sw_tcp_conn_stop calls
iscsi_sw_tcp_release_conn that the recv path has called iscsi_recv_pdu right?
> 3.The kernel passed over this response to user-space which then sent the
> set_neg_params request to kernel
> 4.As the connection was stopped, the sock was NULL and hence while the
> kernel was processing the set param request from user-space, it panicked
>
> Fix
>
> 1.While setting the set_param in kernel, we need to check if sock is NULL
> 2.If the sock is NULL, then return EPERM (operation not permitted)
> 3.Due to this error handler will be invoked in user-space again to
> recover the session
>
> Signed-off-by: Gulam Mohamed
> Reviewed-by: Junxiao Bi
> ---
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index df47557a02a3..fd668a194053 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -711,6 +711,12 @@ static int iscsi_sw_tcp_conn_set_param(struct
> iscsi_cls_conn *cls_conn,
>struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
>struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
>
> + if (!tcp_sw_conn->sock) {
> + iscsi_conn_printk(KERN_ERR, conn,
> + "Cannot set param as sock is NULL\n");
> + return -ENOTCONN;
> + }
> +
I think this might have 2 issues:
1. It only fixes iscsi_tcp. What about other drivers that free/null
resources/fields in ep_disconnect that we layer need to access in the set_param
callback (the cxgbi drivers)?
2. What about drivers that do not free/null fields (be2iscsi, bnx2i, ql4xxx and
qedi) so the set_param calls end up just working. What state will userspace be
in where we have logged in, but iscsid also thinks we are in the middle of
trying to login?
I think we could do:
1. On ep_disconnect and stop_conn set some iscsi_cls_conn bit that indicates
set_param calls for connection level settings should fail. scsi_transport_iscsi
could set and check this bit for all drivers.
2. On bind_conn clear the bit.
Re: [PATCH] scsi: tcmu: Simplify 'tcmu_update_uio_info()'
On 06/16/2019 02:02 AM, Christophe JAILLET wrote:
> Use 'kasprintf()' instead of:
>- snprintf(NULL, 0...
>- kmalloc(...
>- snprintf(...
>
> This is less verbose and saves 7 bytes (i.e. the space for '/(null)') if
> 'udev->dev_config' is NULL.
>
> Signed-off-by: Christophe JAILLET
> ---
> drivers/target/target_core_user.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c
> b/drivers/target/target_core_user.c
> index b43d6385a1a0..04eda111920e 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1824,20 +1824,18 @@ static int tcmu_update_uio_info(struct tcmu_dev *udev)
> {
> struct tcmu_hba *hba = udev->hba->hba_ptr;
> struct uio_info *info;
> - size_t size, used;
> char *str;
>
> info = &udev->uio_info;
> - size = snprintf(NULL, 0, "tcm-user/%u/%s/%s", hba->host_id, udev->name,
> - udev->dev_config);
> - size += 1; /* for \0 */
> - str = kmalloc(size, GFP_KERNEL);
> - if (!str)
> - return -ENOMEM;
>
> - used = snprintf(str, size, "tcm-user/%u/%s", hba->host_id, udev->name);
> if (udev->dev_config[0])
> - snprintf(str + used, size - used, "/%s", udev->dev_config);
> + str = kasprintf(GFP_KERNEL, "tcm-user/%u/%s/%s", hba->host_id,
> + udev->name, udev->dev_config);
> + else
> + str = kasprintf(GFP_KERNEL, "tcm-user/%u/%s", hba->host_id,
> + udev->name);
> + if (!str)
> + return -ENOMEM;
>
> /* If the old string exists, free it */
> kfree(info->name);
>
Thanks.
Acked-by: Mike Christie
Re: [PATCH] nbd: add missed destroy_workqueue when nbd_start_device fails
> On Jul 24, 2020, at 9:27 PM, Li Heng wrote:
>
> destroy_workqueue() should be called to destroy ndev->tx_wq
> when nbd_start_device init resources fails.
>
> Reported-by: Hulk Robot
> Signed-off-by: Li Heng
> ---
> drivers/block/nbd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index ce7e9f22..45e0a9f4 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -4,7 +4,7 @@
> *
> * Note that you can not swap over this thing, yet. Seems to work but
> * deadlocks sometimes - you can not swap over TCP in general.
> - *
> + *
> * Copyright 1997-2000, 2008 Pavel Machek
> * Parts copyright 2001 Steven Whitehouse
> *
> @@ -1270,6 +1270,7 @@ static int nbd_start_device(struct nbd_device *nbd)
> error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
> if (error) {
> dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
> + destroy_workqueue(nbd->recv_workq);
> return error;
> }
> set_bit(NBD_RT_HAS_PID_FILE, &config->runtime_flags);
> @@ -1291,6 +1292,7 @@ static int nbd_start_device(struct nbd_device *nbd)
>*/
> if (i)
> flush_workqueue(nbd->recv_workq);
> + destroy_workqueue(nbd->recv_workq);
> return -ENOMEM;
> }
> sk_set_memalloc(config->socks[i]->sock->sk);
For the netlink error path, we end up cleaning up everything when
nbd_config_put is called in the error path.
Are you seeing an issue with the ioctl interface and this code path? I thought
normally if the the NBD_DO_IT ioctl fails, then userspace closes the device and
that does the nbd_config_put that will clean this up like is done in the
netlink path.
If userspace is not closing the device and is trying to maybe retry the
NBD_DO_IT ioctl or reuse the device some other way, then I think you need to
also NULL nbd->task_recv, remove pid file, NULL recv_workq after you destroy
for the cases nbd_config_put is called right after a failure.
Re: [PATCH] scsi: target: tcmu: Call flush_dcache_page() with proper page struct
> On Jun 19, 2020, at 1:41 PM, Henry Willard wrote:
>
> tcmu_flush_dcache_range() gets called with addresses from both kernel
> linear space and vmalloc space, so virt_to_page() or vmalloc_to_page()
> have to be used as appropriate to get the proper page struct. On x86_64
> flush_dcache_page() is the default noop implementation, so this hasn't
> been a problem there.
>
> When tcmu_flush_dcache_range() is called with a vmalloc address on Arm64,
> the result is a kernel panic with the following stack trace:
>
> [ 448.873342] CPU: 0 PID: 34102 Comm: iscsi_trx Kdump: loaded
> Not tainted 5.4.17-2011.3.2.1.el8uek.aarch64 #2
> [ 448.876144] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> [ 448.878377] pstate: 8045 (Nzcv daif +PAN -UAO)
> [ 448.880182] pc : flush_dcache_page+0x18/0x60
> [ 448.881888] lr : is_ring_space_avail+0x74/0x390 [target_core_user]
> [ 448.883969] sp : 80001720fa70
> [ 448.885450] x29: 80001720fa70 x28:
> [ 448.887348] x27: 0001 x26: 0003c4b88000
> [ 448.889285] x25: 0001 x24: 800017da
> [ 448.891166] x23: ffdfffe0 x22: 0078
> [ 448.893061] x21: 800017da0001 x20:
> [ 448.894931] x19: ffe5f680 x18:
> [ 448.896826] x17: x16:
> [ 448.898704] x15: x14:
> [ 448.900562] x13: x12:
> [ 448.902403] x11: 0003d188e4d0 x10: 0030
> [ 448.904230] x9 : x8 : 0003d4073f00
> [ 448.906094] x7 : 13b0 x6 : 003f
> [ 448.907911] x5 : 0040 x4 : 0003d16d6258
> [ 448.909720] x3 : 0001 x2 : 0078
> [ 448.911664] x1 : 0003d16d6228 x0 : 89f43b1c
> [ 448.913767] Call trace:
> [ 448.914984] flush_dcache_page+0x18/0x60
> [ 448.916518] is_ring_space_avail+0x74/0x390 [target_core_user]
> [ 448.918450] queue_cmd_ring+0x228/0x700 [target_core_user]
> [ 448.920318] tcmu_queue_cmd+0xd8/0x14c [target_core_user]
> [ 448.922192] __target_execute_cmd+0x30/0x130 [target_core_mod]
> [ 448.924170] target_execute_cmd+0x1a4/0x450 [target_core_mod]
> [ 448.926212] transport_generic_new_cmd+0x1b8/0x3a0 [target_core_mod]
> [ 448.928289] transport_handle_cdb_direct+0x50/0xb0 [target_core_mod]
> [ 448.930368] iscsit_execute_cmd+0x2c0/0x360 [iscsi_target_mod]
> [ 448.932347] iscsit_sequence_cmd+0xd8/0x1c8 [iscsi_target_mod]
> [ 448.934313] iscsit_process_scsi_cmd+0xac/0xf8 [iscsi_target_mod]
> [ 448.936479] iscsit_get_rx_pdu+0x450/0xd68 [iscsi_target_mod]
> [ 448.938423] iscsi_target_rx_thread+0xc0/0x168 [iscsi_target_mod]
> [ 448.940387] kthread+0x110/0x114
> [ 448.941802] ret_from_fork+0x10/0x18
> [ 448.943271] Code: f9000bf3 aa0003f3 aa1e03e0 d503201f (f9400260)
> [ 448.945271] SMP: stopping secondary CPUs
>
> Signed-off-by: Henry Willard
> ---
> drivers/target/target_core_user.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_user.c
> b/drivers/target/target_core_user.c
> index 560bfec933bc..7557c0630483 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -597,11 +597,19 @@ static inline void tcmu_flush_dcache_range(void *vaddr,
> size_t size)
> {
> unsigned long offset = offset_in_page(vaddr);
> void *start = vaddr - offset;
> + struct page *pg;
>
> size = round_up(size+offset, PAGE_SIZE);
>
> while (size) {
> - flush_dcache_page(virt_to_page(start));
> + if (virt_addr_valid(start))
> + pg = virt_to_page(start);
> + else if (is_vmalloc_addr(start))
> + pg = vmalloc_to_page(start);
> + else
> + break;
> +
> + flush_dcache_page(pg);
> start += PAGE_SIZE;
This was just fixed by Bodo:
https://lore.kernel.org/linux-scsi/20200618131632.32748-1-bstroes...@ts.fujitsu.com/
Re: [PATCH v3 4/9] vhost: Add kthread support in function vhost_worker_create
On 11/5/24 1:25 AM, Cindy Lu wrote:
> static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> {
> struct vhost_worker *worker;
> - struct vhost_task *vtsk;
> + struct vhost_task *vtsk = NULL;
> + struct task_struct *task = NULL;
> char name[TASK_COMM_LEN];
> int ret;
> u32 id;
>
> + /* Allocate resources for the worker */
> worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> if (!worker)
> return NULL;
>
> + worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> + if (!worker->fn) {
> + kfree(worker);
> + return NULL;
> + }
Why dynamically allocate this?
You could probably even just kill the vhost_task_fn struct since we just
have to the 2 callouts.
> +
> worker->dev = dev;
> snprintf(name, sizeof(name), "vhost-%d", current->pid);
>
> - vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> - worker, name);
> - if (!vtsk)
> - goto free_worker;
> -
> mutex_init(&worker->mutex);
> init_llist_head(&worker->work_list);
> worker->kcov_handle = kcov_common_handle();
> - worker->vtsk = vtsk;
>
> - vhost_task_start(vtsk);
> + if (dev->inherit_owner) {
> + /* Create and start a vhost task */
Maybe instead of this comment and the one below write something about
what inherit_owner means. We can see from the code we are creating a
vhost/kthread, but it's not really obvious why. Something like:
/*
* If inherit_owner is true we use vhost_tasks to create
* the worker so all settings/limits like cgroups, NPROC,
* scheduler, etc are inherited from the owner. If false,
* we use kthreads and only attach to the same cgroups
* as the owner for compat with older kernels.
*/
> + vtsk = vhost_task_create(vhost_run_work_list,
> + vhost_worker_killed, worker, name);
> + if (!vtsk)
> + goto free_worker;
> +
> + worker->vtsk = vtsk;
> + worker->fn->wakeup = vhost_task_wakeup_fn;
> + worker->fn->stop = vhost_task_stop_fn;
> +
> + vhost_task_start(vtsk);
> + } else {
> + /* Create and start a kernel thread */
> + task = kthread_create(vhost_run_work_kthread_list, worker,
> + "vhost-%d", current->pid);
> + if (IS_ERR(task)) {
> + ret = PTR_ERR(task);
> + goto free_worker;
> + }
> + worker->task = task;
> + worker->fn->wakeup = vhost_kthread_wakeup_fn;
> + worker->fn->stop = vhost_kthread_stop_fn;
> +
> + wake_up_process(task);
> + /* Attach to the vhost cgroup */
You don't need this comment do you? The function name tells us the same
info.
> + ret = vhost_attach_cgroups(dev);
I don't think this works. Patch 3/9 did:
+ xa_for_each(&dev->worker_xa, i, worker) {
+ ret = vhost_worker_cgroups_kthread(worker);
but we don't add the worker to the xa until below.
You also want to just call vhost_worker_cgroups_kthread above, because
you only want to add the one task and not loop over all of them.
I would then also maybe rename vhost_worker_cgroups_kthread to something
like vhost_attach_task_to_cgroups.
> + if (ret)
> + goto stop_worker;
> + }
>
> ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> if (ret < 0)
> goto stop_worker;
> worker->id = id;
> -
> return worker;
> -
> stop_worker:
> - vhost_task_stop(vtsk);
> + worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);
I don't think you need to cast since the function takes a void pointer.
Same comment for the other patches like 6/9 where you are calling the
callout and casting.
Re: [PATCH] vhost/scsi: Fix improper cleanup in vhost_scsi_set_endpoint()
On 1/10/25 9:34 PM, Haoran Zhang wrote:
> Since commit 3f8ca2e115e55 ("vhost scsi: alloc cmds per vq instead of
> session"), a bug can be triggered when the host sends a duplicate
> VHOST_SCSI_SET_ENDPOINT ioctl command.
I don't think that git commit is correct. It should be:
25b98b64e284 ("vhost scsi: alloc cmds per vq instead of session")
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 718fa4e0b31e..b994138837f2 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1726,7 +1726,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> mutex_unlock(&tpg->tv_tpg_mutex);
> mutex_unlock(&vhost_scsi_mutex);
> ret = -EEXIST;
> - goto undepend;
> + goto free_vs_tpg;
I agree you found a bug, but I'm not sure how you hit it from here. A
couple lines before this, we do:
if (tpg->tv_tpg_vhost_count != 0) {
mutex_unlock(&tpg->tv_tpg_mutex);
continue;
}
If the tpg was already in the vs_tpg, then tv_tpg_vhost_count would be
non-zero and we would hit the check above.
Could you describe the target and tpg mapping for how you hit this?
> }
> /*
>* In order to ensure individual vhost-scsi configfs
However, I was able to replicate the bug by hitting the chunk below this
comment where we do:
ret = target_depend_item(&se_tpg->tpg_group.cg_item);
if (ret) {
pr_warn("target_depend_item() failed: %d\n", ret);
mutex_unlock(&tpg->tv_tpg_mutex);
mutex_unlock(&vhost_scsi_mutex);
goto undepend;
> @@ -1802,6 +1802,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> target_undepend_item(&tpg->se_tpg.tpg_group.cg_item);
> }
> }
> +free_vs_tpg:
> kfree(vs_tpg);
To fix the bug, I don't think we can just free the vs_tpg. There's 2
cases we can hit the error path:
1. First time calling vhost_scsi_set_endpoint.
If we have a target with 2 tpgs, and for tpg1 we did a successful
target_depend_item call, but then we looped and for tpg2
target_depend_item failed, if we just did a kfree then we would leave a
refcount on tpg1.
So for this case, we need to do the "goto undepend".
2. N > 1 time calling vhost_scsi_set_endpoint.
This one is more complicated because let's say we started with 1 tpg and
on the first call to vhost_scsi_set_endpoint we successfully did
target_depend_item on it. Before the 2nd call to vhost_scsi_set_endpoint
we added tpg2 and tpg3. We then do vhost_scsi_set_endpoint for the 2nd
time, we successfully do target_depend_item on tpg2, but it fails for tpg3.
In this case, we want to unwind what we did on this 2nd call, so we want
to do target_undepend_item on tpg2. And, we don't want to call it for
tpg1 or we will hit the bug you found.
So I think to fix the issue, we would want to:
1. move the
memcpy(vs_tpg, vs->vs_tpg, len);
to the end of the function after we do the vhost_scsi_flush. This will
be more complicated than the current memcpy though. We will want to
merge the local vs_tpg and the vs->vs_tpg like:
for (i = 0; i < VHOST_SCSI_MAX_TARGET; i++) {
if (vs_tpg[i])
vs->vs_tpg[i] = vs_tpg[i])
}
2. We want to leave the "goto undepend" calls as is. For the the
undepend goto handling we also want to leave the code as is. We want to
continue to loop over the local vs_tpg because after we moved the memcpy
for #1 it now only contains the tpgs we updated on the current
vhost_scsi_set_endpoint call.
Re: [PATCH 1/2] vhost-scsi: Fix typos and formatting in comments and logs
On 6/11/25 9:39 AM, Alok Tiwari wrote: > This patch corrects several minor typos and formatting issues. > Changes include: > > Fixing misspellings like in comments > - "explict" -> "explicit" > - "infight" -> "inflight", > - "with generate" -> "will generate" > > formatting in logs > - Correcting log formatting specifier from "%dd" to "%d" > - Adding a missing space in the sysfs emit string to prevent > misinterpreted output like "X86_64on ". changing to "X86_64 on " > - Cleaning up stray semicolons in struct definition endings > > These changes improve code readability and consistency. > no functionality changes. > > Signed-off-by: Alok Tiwari Reviewed-by: Mike Christie
Re: [PATCH 2/2] vhost-scsi: Improve error handling in vhost_scsi_make_nexus and tpg
On 6/11/25 9:39 AM, Alok Tiwari wrote: > Use PTR_ERR to return the actual error code when vhost_scsi_make_nexus > fails to create a session, instead of returning -ENOMEM. > This ensures more accurate error propagation. > > Replace NULL with ERR_PTR(ret) in vhost_scsi_make_tpg to follow kernel > conventions for pointer-returning functions, allowing callers to use > IS_ERR and PTR_ERR for proper error handling. > > Signed-off-by: Alok Tiwari Reviewed-by: Mike Christie
