On Thu, May 19, 2022 at 06:43:06AM +0000, Tian, Kevin wrote:
> > This fixes a user-triggerable oops in GVT.
>
> No changelog.
??
the cover latter clearly states what has changed since v1, and this
patch has a good commit log. This is exactly how it is supposed to
be done.
>
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
>
> Not sure whether Christoph wants a s-o-b here when he wrote
> the snippet to remove the release work of gvt...
That's just tivial code removal, so no.
>
> > @@ -1083,11 +1083,22 @@ static struct file *vfio_device_open(struct
> > vfio_device *device)
> >
> > mutex_lock(&device->dev_set->lock);
> > device->open_count++;
> > + down_read(&device->group->group_rwsem);
> > + if (device->open_count == 1 && device->group->kvm) {
> > + /*
> > + * Here we pass the KVM pointer with the group under the
> > read
> > + * lock. If the device driver will use it, it must obtain a
> > + * reference and release it during close_device.
> > + */
> > + device->kvm = device->group->kvm;
> > + }
> > +
> > if (device->open_count == 1 && device->ops->open_device) {
>
> Merge the two branches so both are under if (device->open_count == 1) {}
> (and group_rwsem can be also moved inside)
Yeah. And we don't really need the device->group->kvm check, as
it would otherwise assign NULL which is perfectly fine.
But otherwise this also looks good to me:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
