netdev - Re: [PATCH v2 6/8] vdpa_sim: use kthread worker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230302154800.z3i4fpjlvtb74efu@sgarzare-redhat>
Date:   Thu, 2 Mar 2023 16:48:00 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     virtualization@...ts.linux-foundation.org,
        Andrey Zhadchenko <andrey.zhadchenko@...tuozzo.com>,
        eperezma@...hat.com, netdev@...r.kernel.org, stefanha@...hat.com,
        linux-kernel@...r.kernel.org, Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>, kvm@...r.kernel.org
Subject: Re: [PATCH v2 6/8] vdpa_sim: use kthread worker

On Thu, Mar 02, 2023 at 04:30:04PM +0100, Simon Horman wrote:
>On Thu, Mar 02, 2023 at 12:34:19PM +0100, Stefano Garzarella wrote:
>> Let's use our own kthread to run device jobs.
>> This allows us more flexibility, especially we can attach the kthread
>> to the user address space when vDPA uses user's VA.
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@...hat.com>
>> ---
>>  drivers/vdpa/vdpa_sim/vdpa_sim.h |  3 ++-
>>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 17 ++++++++++++-----
>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> index acee20faaf6a..ce83f9130a5d 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> @@ -57,7 +57,8 @@ struct vdpasim_dev_attr {
>>  struct vdpasim {
>>  	struct vdpa_device vdpa;
>>  	struct vdpasim_virtqueue *vqs;
>> -	struct work_struct work;
>> +	struct kthread_worker *worker;
>> +	struct kthread_work work;
>>  	struct vdpasim_dev_attr dev_attr;
>>  	/* spinlock to synchronize virtqueue state */
>>  	spinlock_t lock;
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> index a6ee830efc38..6feb29726c2a 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> @@ -11,8 +11,8 @@
>>  #include <linux/module.h>
>>  #include <linux/device.h>
>>  #include <linux/kernel.h>
>> +#include <linux/kthread.h>
>>  #include <linux/slab.h>
>> -#include <linux/sched.h>
>>  #include <linux/dma-map-ops.h>
>>  #include <linux/vringh.h>
>>  #include <linux/vdpa.h>
>> @@ -116,7 +116,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
>>  static const struct vdpa_config_ops vdpasim_config_ops;
>>  static const struct vdpa_config_ops vdpasim_batch_config_ops;
>>
>> -static void vdpasim_work_fn(struct work_struct *work)
>> +static void vdpasim_work_fn(struct kthread_work *work)
>>  {
>>  	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
>>
>> @@ -159,7 +159,13 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
>>
>>  	vdpasim = vdpa_to_sim(vdpa);
>>  	vdpasim->dev_attr = *dev_attr;
>> -	INIT_WORK(&vdpasim->work, vdpasim_work_fn);
>> +
>> +	kthread_init_work(&vdpasim->work, vdpasim_work_fn);
>> +	vdpasim->worker = kthread_create_worker(0, "vDPA sim worker: %s",
>> +						dev_attr->name);
>> +	if (IS_ERR(vdpasim->worker))
>> +		goto err_iommu;
>
>Branching to err_iommu will result in a call to put_device(dev)...

Good catch!

>
>> +
>>  	spin_lock_init(&vdpasim->lock);
>>  	spin_lock_init(&vdpasim->iommu_lock);
>
>... but dev is not initialised until the line following this hunk,
>which is:
>
>	dev = &vdpasim->vdpa.dev;
>
>In order to avoid leaking dev I _think_ the correct approach
>is to move the initialisation of dev above the branch to
>err_iommu, perhaps above the call to kthread_init_work()
>is a good place.

Yep, I agree. I'll fix in v3.

Thanks,
Stefano

>
>This does move the assignment outside the locks above.
>But I _think_ that is ok.
>
>> @@ -212,7 +218,7 @@ EXPORT_SYMBOL_GPL(vdpasim_create);
>>
>>  void vdpasim_schedule_work(struct vdpasim *vdpasim)
>>  {
>> -	schedule_work(&vdpasim->work);
>> +	kthread_queue_work(vdpasim->worker, &vdpasim->work);
>>  }
>>  EXPORT_SYMBOL_GPL(vdpasim_schedule_work);
>>
>> @@ -612,7 +618,8 @@ static void vdpasim_free(struct vdpa_device *vdpa)
>>  	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>  	int i;
>>
>> -	cancel_work_sync(&vdpasim->work);
>> +	kthread_cancel_work_sync(&vdpasim->work);
>> +	kthread_destroy_worker(vdpasim->worker);
>>
>>  	for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
>>  		vringh_kiov_cleanup(&vdpasim->vqs[i].out_iov);
>> --
>> 2.39.2
>>
>