Re: Report a possible vhost bug in stable branches

* Re: Report a possible vhost bug in stable branches
       [not found] <a8dadf64-7723-926d-290f-945df04df194@linux.alibaba.com>
@ 2023-10-12  7:55 ` Jason Wang
       [not found]   ` <b51b8498-c7de-2946-fee2-beee058e1e41@linux.alibaba.com>
  0 siblings, 1 reply; 2+ messages in thread
From: Jason Wang @ 2023-10-12  7:55 UTC (permalink / raw)
  To: Xianting Tian; +Cc: virtualization, Michael S. Tsirkin

On Thu, Oct 12, 2023 at 9:43 AM Xianting Tian
<xianting.tian@linux.alibaba.com> wrote:
>
> cgroup attach work and dev flush work will both be added to dev work
> list in vhost_attach_cgroups() when set dev owner:
>              static int vhost_attach_cgroups(struct vhost_dev *dev)
>              {
>                      struct vhost_attach_cgroups_struct attach;
>
>                      attach.owner = current;
>                      vhost_work_init(&attach.work,
>                                     vhost_attach_cgroups_work);
>                      vhost_work_queue(dev, &attach.work); // add cgroup
> attach work
>                      vhost_work_dev_flush(dev);           // add dev
> flush work
>                      return attach.ret;
>              }
>
>    And dev kworker will be waken up to handle the two works in
> vhost_worker():
>              node = llist_del_all(&dev->work_list);
>              node = llist_reverse_order(node);
>              llist_for_each_entry_safe{
>                      work->fn(work);
>              }
>
>    As the list is reversed before processing in vhost_worker(), so it is
> possible
>    that dev flush work is processed before cgroup attach work.

This sounds weird. It's llist not list so when adding the new entry
was added to the head that why we need llist_reverse_order() to
recover the order.

 Have you ever reproduced these issues?

Thanks

> If so,
> vhost_attach_cgroups
>    may return "attach.ret" before cgroup attach work is handled, but
> "attach.ret" is random
>    value as it is in stack.
>
> The possible fix maybe:
>
> static int vhost_attach_cgroups(struct vhost_dev *dev)
> {
>          struct vhost_attach_cgroups_struct attach;
>
>          attach.ret = 0;
>          attach.owner = current;
>          vhost_work_init(&attach.work, vhost_attach_cgroups_work);
>          vhost_work_queue(dev, &attach.work);
>          vhost_work_dev_flush(dev);
>          return attach.ret;
> }
>
>   So this fix is just to initialize the attach.ret to 0, this fix may
> not the final fix,
>   We just want you experts know this issue exists, and we met it
> recently in our test.
>
> And the issue exists in may stable branches.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 2+ messages in thread