linux-kernel - Re: [PATCH v4 bpf-next 4/6] bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs

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

Message-ID: <CAP01T76SVQ=TJgkTgkvSLY3DFTDUswj_aypAWmQhwKWFBEk_yw@mail.gmail.com>
Date: Tue, 17 Dec 2024 19:32:21 +0100
From: Kumar Kartikeya Dwivedi <memxor@...il.com>
To: Song Liu <songliubraving@...a.com>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Song Liu <song@...nel.org>, 
	bpf <bpf@...r.kernel.org>, Linux-Fsdevel <linux-fsdevel@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, 
	LSM List <linux-security-module@...r.kernel.org>, Kernel Team <kernel-team@...a.com>, 
	Andrii Nakryiko <andrii@...nel.org>, Eddy Z <eddyz87@...il.com>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, KP Singh <kpsingh@...nel.org>, 
	Matt Bobrowski <mattbobrowski@...gle.com>, Liam Wisehart <liamwisehart@...a.com>, 
	Shankaran Gnanashanmugam <shankaran@...a.com>
Subject: Re: [PATCH v4 bpf-next 4/6] bpf: fs/xattr: Add BPF kfuncs to set and
 remove xattrs

On Tue, 17 Dec 2024 at 19:25, Song Liu <songliubraving@...a.com> wrote:
>
> Hi Alexei,
>
> Thanks for the review!
>
> > On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> >
> > On Mon, Dec 16, 2024 at 10:38 PM Song Liu <song@...nel.org> wrote:
> >>
> >> Add the following kfuncs to set and remove xattrs from BPF programs:
> >>
> >>  bpf_set_dentry_xattr
> >>  bpf_remove_dentry_xattr
> >>  bpf_set_dentry_xattr_locked
> >>  bpf_remove_dentry_xattr_locked
> >>
> >> The _locked version of these kfuncs are called from hooks where
> >> dentry->d_inode is already locked.
> >
> > ...
> >
> >> + *
> >> + * Setting and removing xattr requires exclusive lock on dentry->d_inode.
> >> + * Some hooks already locked d_inode, while some hooks have not locked
> >> + * d_inode. Therefore, we need different kfuncs for different hooks.
> >> + * Specifically, hooks in the following list (d_inode_locked_hooks)
> >> + * should call bpf_[set|remove]_dentry_xattr_locked; while other hooks
> >> + * should call bpf_[set|remove]_dentry_xattr.
> >> + */
> >
> > the inode locking rules might change, so let's hide this
> > implementation detail from the bpf progs by making kfunc polymorphic.
> >
> > To struct bpf_prog_aux add:
> > bool use_locked_kfunc:1;
> > and set it in bpf_check_attach_target() if it's attaching
> > to one of d_inode_locked_hooks
> >
> > Then in fixup_kfunc_call() call some helper that
> > if (prog->aux->use_locked_kfunc &&
> >    insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr])
> >     insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked];
> >
> > The progs will be simpler and will suffer less churn
> > when the kernel side changes.
>
> I was thinking about something in similar direction.
>
> If we do this, shall we somehow hide the _locked version of the
> kfuncs, so that the user cannot use it? If so, what's the best
> way to do it?

Just don't add BTF_ID_FLAGS entries for them.
You'd also need to make an extra call to add_kfunc_call to add its
details before you can do the fixup.
That allows find_kfunc_desc to work.
I did something similar in earlier versions of resilient locks.
In add_kfunc_call's end (instead of directly returning):
func_id = get_shadow_kfunc_id(func_id, offset);
if (!func_id)
  return err;
return add_kfunc_call(env, func_id, offset);

Then check in fixup_kfunc_call to find shadow kfunc id and substitute imm.
Can use some other naming instead of "shadow".
Probably need to take a prog pointer to make a decision to find the
underlying kfunc id in your case.

>
> Thanks,
> Song
>