Re: [PATCH RFC 2/4] sysctl: Apply sysctl context when flag is active

[Alexey Gladkov <legion@kernel.org>]

On Thu, Sep 24, 2026 at 04:34:10PM +0200, Joel Granados wrote:
> Whenever CTL_TABLE_F_CTX_DATA is set on the entry, overlay the ->data
> entry contained in inst on the entry to be passed to proc_handlers. No
> table sets the flags yet, so there is no functional change.
> 
> Signed-off-by: Joel Granados <joel.granados@kernel.org>
> ---
>  fs/proc/proc_sysctl.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index fe32337892f9badc897a29e9176e66790003fb05..1ed9f4e1a17e28c5db1d07356390cbee968cb91f 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -550,12 +550,31 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>  	return err;
>  }
>  
> +/* Returns @entry itself when it is not flagged, else a resolved copy in @buf. */
> +static const struct ctl_table *sysctl_apply_ctx(struct ctl_table_header *head,
> +						const struct ctl_table *entry,
> +						struct ctl_table *buf)
> +{
> +	ptrdiff_t off;
> +
> +	if (!(entry->flags & CTL_TABLE_F_CTX_DATA))
> +		return entry;
> +
> +	off = (const char *)entry->data - (const char *)head->ctx.tmpl;
> +
> +	*buf = *entry;
> +	buf->data = (char *)head->ctx.inst + off;
> +
> +	return buf;
> +}

I had to create a more complex version of the context because,
unfortunately, yours approach doesn't cover all current use cases and does
not check the variable's type based on this offset.

In my RFC [1] I showed Linus, I included three illustrative examples (a
simple one, a worse one, and the ugliest one). In the net/mpls/af_mpls.c
we need to have netns and struct mpls_dev in same time [2]. A namespace
alone isn't enough for us. Unfortunately, this isn't the only place where
this approach is used.

Also, I really wanted to avoid dealing with void pointer arithmetic
as much as possible.

A long time ago, Linus had already pointed out to me that having
"void *ctl_data" was a bad solution [3]:

  There is no reason to have some pseudo-generic "void *ctl_data" that
  makes it ambiguous and allows for type confusion and isn't
  self-documenting. I'd rather have a properly typed pointer that is
  just initialized to NULL and is not always used or needed, but always
  has a clear case for *what* it would be used for.
  
  Yes, yes, we have f_private etc for things that are really very very
  generic and have arbitrary users. But 'sysctl' is not that kind of
  truly generic use.

Yes, my version still has similar code, but the offset is calculated at
compile time and a type check is made.

I like my version better because I've checked all the places where the
ctl_table list is cloned, and I know for sure that my version will be able
to handle all those use cases. And, you known, I wrote it :)

But seriously, don't get me wrong, I'm totally fine if you continue with
your version. The main thing is that it should cover all existing
use cases.

[1] https://lore.kernel.org/linux-fsdevel/cover.1788018958.git.legion@kernel.org/
[2] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mpls/af_mpls.c#n1429
[3] https://lore.kernel.org/all/b0ccbb2489119f1f20c737cf1930c3a9c4e4243a.1644862280.git.legion@kernel.org/T/#m5d0b462c317f9cd0e772b5df60ec8ceba41dcac0

> +
>  static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>  		int write)
>  {
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	struct ctl_table_header *head = grab_header(inode);
>  	const struct ctl_table *table = PROC_I(inode)->sysctl_entry;
> +	struct ctl_table ctx_entry;
>  	size_t count = iov_iter_count(iter);
>  	char *kbuf;
>  	ssize_t error;
> @@ -567,6 +586,8 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>  	 * At this point we know that the sysctl was not unregistered
>  	 * and won't be until we finish.
>  	 */
> +	table = sysctl_apply_ctx(head, table, &ctx_entry);
> +
>  	error = -EPERM;
>  	if (sysctl_perm(head, table, write ? MAY_WRITE : MAY_READ))
>  		goto out;
> @@ -797,6 +818,7 @@ static int proc_sys_permission(struct mnt_idmap *idmap,
>  	 */
>  	struct ctl_table_header *head;
>  	const struct ctl_table *table;
> +	struct ctl_table ctx_entry;
>  	int error;
>  
>  	/* Executable files are not allowed under /proc/sys/ */
> @@ -810,8 +832,11 @@ static int proc_sys_permission(struct mnt_idmap *idmap,
>  	table = PROC_I(inode)->sysctl_entry;
>  	if (!table) /* global root - r-xr-xr-x */
>  		error = mask & MAY_WRITE ? -EACCES : 0;
> -	else /* Use the permissions on the sysctl table entry */
> +	else {
> +		/* ->permissions may inspect the entry, so resolve it first. */
> +		table = sysctl_apply_ctx(head, table, &ctx_entry);
>  		error = sysctl_perm(head, table, mask & ~MAY_NOT_BLOCK);
> +	}
>  
>  	sysctl_head_finish(head);
>  	return error;
> 
> -- 
> 2.50.1
> 
> 

-- 
Rgrds, legion