Re: [PATCH v4 5/7] revocable: Add fops replacement

[<dan.j.williams@intel.com>]

Tzung-Bi Shih wrote:
> Introduce revocable_replace_fops() to simplify the use of the revocable
> API with file_operations.
> 
> The function, should be called from a driver's ->open(), replaces the
> fops with a wrapper that automatically handles the `try_access` and
> `withdraw_access`.
> 
> When the file is closed, the wrapper's ->release() restores the original
> fops and cleanups.  This centralizes the revocable logic, making drivers
> cleaner and easier to maintain.
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
[..]
> +/**
> + * revocable_replace_fops() - Replace the file operations to be revocable-aware.
> + *
> + * Should be used only from ->open() instances.
> + */
> +int revocable_replace_fops(struct file *filp, struct revocable_provider *rp,
> +			   const struct revocable_operations *rops)
> +{
> +	struct fops_replacement *fr;
> +
> +	fr = kzalloc(sizeof(*fr), GFP_KERNEL);
> +	if (!fr)
> +		return -ENOMEM;
> +
> +	fr->filp = filp;
> +	fr->rops = rops;
> +	fr->orig_fops = filp->f_op;
> +	fr->rev = revocable_alloc(rp);
> +	if (!fr->rev)
> +		return -ENOMEM;
> +	memcpy(&fr->fops, filp->f_op, sizeof(struct file_operations));
> +	scoped_guard(mutex, &fops_replacement_mutex)
> +		list_add(&fr->list, &fops_replacement_list);

This list grows for every active instance? Unless I am misreading, that
looks like a scaling burden that the simple approach below does not
have.

> +	fr->fops.release = revocable_fr_release;
> +
> +	if (filp->f_op->read)
> +		fr->fops.read = revocable_fr_read;
> +	if (filp->f_op->poll)
> +		fr->fops.poll = revocable_fr_poll;
> +	if (filp->f_op->unlocked_ioctl)
> +		fr->fops.unlocked_ioctl = revocable_fr_unlocked_ioctl;
> +
> +	filp->f_op = &fr->fops;
> +	return 0;
> +}

This facility is protecting the wrong resource, and I argue hides bugs
in drivers that think they need this. That matches the conclusion I came
to with my "managed_fops" attempt.

The resource that is being revoked is the device's attachment to its
driver. Whether that is dev_get_drvdata() or some other device-to-data
lookup, that is the resource that gets removed, not the fops themselves.
The only resource race with fops is whether the code text section
remains available while the fops are registered, but that lifetime scope
is not at a per-device instance scope.

revocable() could be useful for more complicated scenarios, but for
making sure that ->unlocked_ioctl(), ->poll(), and ->read() start
reliably failing, just do:

    guard(rwsem_read)(&subsystem_device_registration_lock);
    data = subsystem_lookup_device_relative_data(...)
    if (data)
        return subsystem_{ioctl,poll,read}(data, ...);
    return -ENXIO;

...and revoke future subsystem_{ioctl,poll,read}() invocations by making
subsystem_lookup_device_relative_data() start failing under
guard(rwsem_write)(&subsystem_device_registration_lock).