netdev - Re: [PATCH V4 mlx5-next 13/13] vfio/mlx5: Use its own PCI reset

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: <20211026235002.GC2744544@nvidia.com>
Date:   Tue, 26 Oct 2021 20:50:02 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Yishai Hadas <yishaih@...dia.com>, bhelgaas@...gle.com,
        saeedm@...dia.com, linux-pci@...r.kernel.org, kvm@...r.kernel.org,
        netdev@...r.kernel.org, kuba@...nel.org, leonro@...dia.com,
        kwankhede@...dia.com, mgurtovoy@...dia.com, maorg@...dia.com
Subject: Re: [PATCH V4 mlx5-next 13/13] vfio/mlx5: Use its own PCI reset_done
 error handler

On Tue, Oct 26, 2021 at 05:16:44PM -0600, Alex Williamson wrote:
> > @@ -471,6 +474,47 @@ mlx5vf_pci_migration_data_rw(struct mlx5vf_pci_core_device *mvdev,
> >  	return count;
> >  }
> >  
> > +/* This function is called in all state_mutex unlock cases to
> > + * handle a 'defered_reset' if exists.
> > + */
> 
> I refrained from noting it elsewhere, but we're not in net/ or
> drivers/net/ here, but we're using their multi-line comment style.  Are
> we using the strong relation to a driver that does belong there as
> justification for the style here?

I think it is an oversight, tell Yishai you prefer the other format in
drivers/vfio and it can be fixed

> > @@ -539,7 +583,7 @@ static ssize_t mlx5vf_pci_mig_rw(struct vfio_pci_core_device *vdev,
> >  	}
> >  
> >  end:
> > -	mutex_unlock(&mvdev->state_mutex);
> > +	mlx5vf_state_mutex_unlock(mvdev);
> 
> I'm a little lost here, if the operation was to read the device_state
> and mvdev->vmig.vfio_dev_state was error, that's already been copied to
> the user buffer, so the user continues to see the error state for the
> first read of device_state after reset if they encounter this race?

Yes. If the userspace races ioctls they get a deserved mess.

This race exists no matter what we do, as soon as the unlock happens a
racing reset ioctl could run in during the system call exit path.

The purpose of the locking is to protect the kernel from hostile
userspace, not to allow userspace to execute concurrent ioctl's in a
sensible way.

Jason