Re: [PATCH v2 7/8] fwctl/mlx5: Support for communicating with mlx5 fw

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jul 26, 2024 at 05:10:13PM +0100, Jonathan Cameron wrote:

> > diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> > index 37147a695add9a..e5ee2d46d43126 100644
> > --- a/drivers/fwctl/Kconfig
> > +++ b/drivers/fwctl/Kconfig
> > @@ -7,3 +7,17 @@ menuconfig FWCTL
> >  	  support a wide range of lockdown compatible device behaviors including
> >  	  manipulating device FLASH, debugging, and other activities that don't
> >  	  fit neatly into an existing subsystem.
> > +
> > +if FWCTL
> 
> Why not use depends on FWCTL?

This is a "safer" pattern for kconfig if you expect a list of
drivers. You put all the driver kconfig stanza's within the above if
and then they all pick it up correctly and consistently. Otherwise you
have to replicate the depends line.

> > +static void mlx5ctl_remove(struct auxiliary_device *adev)
> > +{
> > +	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);
> 
> So this is calling fwctl_put(&mcdev->fwctl) on scope exit.
> 
> Why do you need to drop a reference beyond the one fwctl_unregister() is dropping
> in cdev_device_del()?  Where am I missing a reference get?

fwctl_register() / fwctl_unregister() are pairs. Internally they pair
cdev_device_add() / cdev_device_del() which decrease some internal
cdev refcounts.

_alloc_device() / __free(mlx5ctl) above are the other pair.
device_initialize() holds a reference from probe to remove.

It has to work this way because if cdev_device_del() would put back
all the references we would immediately UAF, eg:

	cdev_device_del(&fwctl->cdev, &fwctl->dev);

	/* Disable and free the driver's resources for any still open FDs. */
	guard(rwsem_write)(&fwctl->registration_lock);
	guard(mutex)(&fwctl->uctx_list_lock);
                    ^^^^^^^
                       Must still be allocated

And more broadly, though mlx5 does not use this, it would be safe for
a driver to do:

    fwctl_unregister();
    kfree(mcdev->mymemory);
          ^^^^^^ Must still be allocated!
    fwctl_put(&mcdev->fwctl);

So we have the two steps where unregister makes it safe for the driver
to begin teardown but keeps memory around, and the final put which
releases the memory after driver teardown is done.

This is also captured in the cleanup.h notation:

	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = fwctl_alloc_device(
		&mdev->pdev->dev, &mlx5ctl_ops, struct mlx5ctl_dev,
		fwctl);
                                  ^^^^^^^^^^^^
               Here we indicate we have a ref on the stack from
               fwctl_alloc_device

	auxiliary_set_drvdata(adev, no_free_ptr(mcdev));
                                    ^^^^^^^^^^^^^^^^^ Move the ref
				    into drvdata

	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);
                                    ^^^^^^^^^^^ Move the ref out of
				    drvdata onto the stack

> > +static const struct auxiliary_device_id mlx5ctl_id_table[] = {
> > +	{.name = MLX5_ADEV_NAME ".fwctl",},
> > +	{},
> 
> No point in comma after terminating entries

Sure

Jason