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

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] [day] [month] [year] [list]

Message-ID: <aN05TFvhPPj5voUD@tzungbi-laptop>
Date: Wed, 1 Oct 2025 22:23:09 +0800
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: dan.j.williams@...el.com
Cc: Benson Leung <bleung@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
	Dawid Niedzwiecki <dawidn@...gle.com>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
	linux-kselftest@...r.kernel.org,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH v4 5/7] revocable: Add fops replacement

On Wed, Sep 24, 2025 at 01:54:14PM -0700, dan.j.williams@...el.com wrote:
> Tzung-Bi Shih wrote:
> > +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.

Correct, unless we want to embed the context (e.g. struct fops_replacement)
into struct file.  FWIW: the issue also listed as a known issue after "---".

> > +	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_replace_fops() doesn't protect any resources.  It replaces the
fops to revocable wrappers and recovers the fops when the file is releasing.