Re: [PATCH v5 03/13] iommufd: Add iommufd_verify_unfinalized_object

2024-10-30 Thread Jason Gunthorpe
On Tue, Oct 29, 2024 at 09:05:14PM -0700, Nicolin Chen wrote:
> On Tue, Oct 29, 2024 at 03:55:58PM -0300, Jason Gunthorpe wrote:
> > On Tue, Oct 29, 2024 at 09:18:05AM -0700, Nicolin Chen wrote:
> > > I think we'd need the same change in iommufd_object_abort() too.
> > 
> > Makes sense
> 
> I found xa_cmpxchg() does xas_result to its returning value, which
> turns XA_ZERO_ENTRY into NULL failing our intended verifications.

Oh.. that is annoying, you can't actually tell if cmpxchg failed :\
NULL means success if it was XA_ZERO_ENTRY and failure of it was not
populated! Hmm, I might ask Matthew about this

Your version looks OK

Jason



Re: [PATCH v5 03/13] iommufd: Add iommufd_verify_unfinalized_object

2024-10-29 Thread Nicolin Chen
On Tue, Oct 29, 2024 at 03:55:58PM -0300, Jason Gunthorpe wrote:
> On Tue, Oct 29, 2024 at 09:18:05AM -0700, Nicolin Chen wrote:
> > I think we'd need the same change in iommufd_object_abort() too.
> 
> Makes sense

I found xa_cmpxchg() does xas_result to its returning value, which
turns XA_ZERO_ENTRY into NULL failing our intended verifications.

So, I replaced that further with xas_store:
-
@@ -41,20 +41,26 @@ static struct miscdevice vfio_misc_dev;
 void iommufd_object_finalize(struct iommufd_ctx *ictx,
 struct iommufd_object *obj)
 {
+   XA_STATE(xas, &ictx->objects, obj->id);
void *old;

-   old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
-   /* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
-   WARN_ON(old);
+   xa_lock(&ictx->objects);
+   old = xas_store(&xas, obj);
+   xa_unlock(&ictx->objects);
+   /* obj->id was returned from xa_alloc() so the xas_store() cannot fail 
*/
+   WARN_ON(old != XA_ZERO_ENTRY);
 }

 /* Undo _iommufd_object_alloc() if iommufd_object_finalize() was not called */
 void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj)
 {
+   XA_STATE(xas, &ictx->objects, obj->id);
void *old;

-   old = xa_erase(&ictx->objects, obj->id);
-   WARN_ON(old);
+   xa_lock(&ictx->objects);
+   old = xas_store(&xas, NULL);
+   xa_unlock(&ictx->objects);
+   WARN_ON(old != XA_ZERO_ENTRY);
kfree(obj);
 }
-

Thanks
Nicolin



Re: [PATCH v5 03/13] iommufd: Add iommufd_verify_unfinalized_object

2024-10-29 Thread Jason Gunthorpe
On Tue, Oct 29, 2024 at 09:18:05AM -0700, Nicolin Chen wrote:
> On Tue, Oct 29, 2024 at 11:49:07AM -0300, Jason Gunthorpe wrote:
> > On Fri, Oct 25, 2024 at 04:49:43PM -0700, Nicolin Chen wrote:
> > > To support driver-allocated vIOMMU objects, it's suggested to call the
> > > allocator helper in IOMMU dirvers. However, there is no guarantee that
> > > drivers will all use it and allocate objects properly.
> > > 
> > > Add a helper for iommufd core to verify if an unfinalized object is at
> > > least reserved in the ictx.
> > 
> > I don't think we need this..
> > 
> > iommufd_object_finalize() already does:
> > 
> > old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
> > /* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
> > WARN_ON(old);
> 
> It feels unsafe to carry on the iommufd_viommu_alloc_ioctl() until
> iommufd_object_finalize() as the function would touch the returned
> faulty viommu pointer? E.g. what if the viommu has an even smaller
> size than struct iommufd_viommu?

This is Linux just because the output came from a driver doesn't mean
we have to validate it somehow. It is reasonable to be helpful and
detect driver bugs, but if the driver is buggy it is still OK to
crash.

So you don't *have* to check any of this, if the driver didn't use the
right function to allocate the memory then it will go bad pretty fast.

Improving the xa_store() is something that will detect more kinds of
bugs everywhere, so seems more worthwhile

> I think we'd need the same change in iommufd_object_abort() too.

Makes sense

Jason



Re: [PATCH v5 03/13] iommufd: Add iommufd_verify_unfinalized_object

2024-10-29 Thread Nicolin Chen
On Tue, Oct 29, 2024 at 03:55:58PM -0300, Jason Gunthorpe wrote:
> On Tue, Oct 29, 2024 at 09:18:05AM -0700, Nicolin Chen wrote:
> > On Tue, Oct 29, 2024 at 11:49:07AM -0300, Jason Gunthorpe wrote:
> > > On Fri, Oct 25, 2024 at 04:49:43PM -0700, Nicolin Chen wrote:
> > > > To support driver-allocated vIOMMU objects, it's suggested to call the
> > > > allocator helper in IOMMU dirvers. However, there is no guarantee that
> > > > drivers will all use it and allocate objects properly.
> > > > 
> > > > Add a helper for iommufd core to verify if an unfinalized object is at
> > > > least reserved in the ictx.
> > > 
> > > I don't think we need this..
> > > 
> > > iommufd_object_finalize() already does:
> > > 
> > >   old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
> > >   /* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
> > >   WARN_ON(old);
> > 
> > It feels unsafe to carry on the iommufd_viommu_alloc_ioctl() until
> > iommufd_object_finalize() as the function would touch the returned
> > faulty viommu pointer? E.g. what if the viommu has an even smaller
> > size than struct iommufd_viommu?
> 
> This is Linux just because the output came from a driver doesn't mean
> we have to validate it somehow. It is reasonable to be helpful and
> detect driver bugs, but if the driver is buggy it is still OK to
> crash.
> 
> So you don't *have* to check any of this, if the driver didn't use the
> right function to allocate the memory then it will go bad pretty fast.
> 
> Improving the xa_store() is something that will detect more kinds of
> bugs everywhere, so seems more worthwhile

I see. Thanks!

Nicolin



Re: [PATCH v5 03/13] iommufd: Add iommufd_verify_unfinalized_object

2024-10-29 Thread Nicolin Chen
On Tue, Oct 29, 2024 at 11:49:07AM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 25, 2024 at 04:49:43PM -0700, Nicolin Chen wrote:
> > To support driver-allocated vIOMMU objects, it's suggested to call the
> > allocator helper in IOMMU dirvers. However, there is no guarantee that
> > drivers will all use it and allocate objects properly.
> > 
> > Add a helper for iommufd core to verify if an unfinalized object is at
> > least reserved in the ictx.
> 
> I don't think we need this..
> 
> iommufd_object_finalize() already does:
> 
>   old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
>   /* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
>   WARN_ON(old);

It feels unsafe to carry on the iommufd_viommu_alloc_ioctl() until
iommufd_object_finalize() as the function would touch the returned
faulty viommu pointer? E.g. what if the viommu has an even smaller
size than struct iommufd_viommu?

> So, we could enhance it to be more robust, like this patch is doing:
> 
> void iommufd_object_finalize(struct iommufd_ctx *ictx,
>struct iommufd_object *obj)
> {
>   void *old;
> 
>   old = xa_cmpxchg(&ictx->objects, obj->id, XA_ZERO_ENTRY, obj,
>GFP_KERNEL);
>   /* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
>   WARN_ON(old != XA_ZERO_ENTRY);
> 
> It is always a driver bug to not initialize that object, so WARN_ON is
> OK.

I think we'd need the same change in iommufd_object_abort() too.

Thanks
Nicolin



Re: [PATCH v5 03/13] iommufd: Add iommufd_verify_unfinalized_object

2024-10-29 Thread Jason Gunthorpe
On Fri, Oct 25, 2024 at 04:49:43PM -0700, Nicolin Chen wrote:
> To support driver-allocated vIOMMU objects, it's suggested to call the
> allocator helper in IOMMU dirvers. However, there is no guarantee that
> drivers will all use it and allocate objects properly.
> 
> Add a helper for iommufd core to verify if an unfinalized object is at
> least reserved in the ictx.

I don't think we need this..

iommufd_object_finalize() already does:

old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
/* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
WARN_ON(old);

So, we could enhance it to be more robust, like this patch is doing:

void iommufd_object_finalize(struct iommufd_ctx *ictx,
 struct iommufd_object *obj)
{
void *old;

old = xa_cmpxchg(&ictx->objects, obj->id, XA_ZERO_ENTRY, obj,
 GFP_KERNEL);
/* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
WARN_ON(old != XA_ZERO_ENTRY);

It is always a driver bug to not initialize that object, so WARN_ON is
OK.

Jason