netdev - Re: [PATCH 17/39] bpf: resolve_pseudo_ldimm64(): take handling of a single ldimm64 insn into helper

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: <CAEf4BzaeFTn41pP_hbcrCTKNZjwt3TPojv0_CYbP=+973YnWiA@mail.gmail.com>
Date: Wed, 7 Aug 2024 08:30:29 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: viro@...nel.org, bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	amir73il@...il.com, cgroups@...r.kernel.org, kvm@...r.kernel.org, 
	netdev@...r.kernel.org, torvalds@...ux-foundation.org
Subject: Re: [PATCH 17/39] bpf: resolve_pseudo_ldimm64(): take handling of a
 single ldimm64 insn into helper

On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner <brauner@...nel.org> wrote:
>
> On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> > On Mon, Jul 29, 2024 at 10:20 PM <viro@...nel.org> wrote:
> > >
> > > From: Al Viro <viro@...iv.linux.org.uk>
> > >
> > > Equivalent transformation.  For one thing, it's easier to follow that way.
> > > For another, that simplifies the control flow in the vicinity of struct fd
> > > handling in there, which will allow a switch to CLASS(fd) and make the
> > > thing much easier to verify wrt leaks.
> > >
> > > Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> > > ---
> > >  kernel/bpf/verifier.c | 342 +++++++++++++++++++++---------------------
> > >  1 file changed, 172 insertions(+), 170 deletions(-)
> > >
> >
> > This looks unnecessarily intrusive. I think it's best to extract the
> > logic of fetching and adding bpf_map by fd into a helper and that way
> > contain fdget + fdput logic nicely. Something like below, which I can
> > send to bpf-next.
> >
> > commit b5eec08241cc0263e560551de91eda73ccc5987d
> > Author: Andrii Nakryiko <andrii@...nel.org>
> > Date:   Tue Aug 6 14:31:34 2024 -0700
> >
> >     bpf: factor out fetching bpf_map from FD and adding it to used_maps list
> >
> >     Factor out the logic to extract bpf_map instances from FD embedded in
> >     bpf_insns, adding it to the list of used_maps (unless it's already
> >     there, in which case we just reuse map's index). This simplifies the
> >     logic in resolve_pseudo_ldimm64(), especially around `struct fd`
> >     handling, as all that is now neatly contained in the helper and doesn't
> >     leak into a dozen error handling paths.
> >
> >     Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index df3be12096cf..14e4ef687a59 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -18865,6 +18865,58 @@ static bool bpf_map_is_cgroup_storage(struct
> > bpf_map *map)
> >          map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
> >  }
> >
> > +/* Add map behind fd to used maps list, if it's not already there, and return
> > + * its index. Also set *reused to true if this map was already in the list of
> > + * used maps.
> > + * Returns <0 on error, or >= 0 index, on success.
> > + */
> > +static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd,
> > bool *reused)
> > +{
> > +    struct fd f = fdget(fd);
>
> Use CLASS(fd, f)(fd) and you can avoid all that fdput() stuff.

That was the point of Al's next patch in the series, so I didn't want
to do it in this one that just refactored the logic of adding maps.
But I can fold that in and send it to bpf-next.