linux-kernel - Re: [PATCH v12 02/10] btf: Handle dynamic pointer parameter in kfuncs

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: <bb4bdd90017d5772bdc31dfac93f2e86c6c61b82.camel@huaweicloud.com>
Date:   Fri, 26 Aug 2022 17:34:57 +0200
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Mykola Lysenko <mykolal@...com>,
        Jonathan Corbet <corbet@....net>,
        David Howells <dhowells@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Paul Moore <paul@...l-moore.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Shuah Khan <shuah@...nel.org>, bpf <bpf@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        keyrings@...r.kernel.org,
        LSM List <linux-security-module@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Daniel Müller <deso@...teo.net>,
        Roberto Sassu <roberto.sassu@...wei.com>,
        Joanne Koong <joannelkoong@...il.com>
Subject: Re: [PATCH v12 02/10] btf: Handle dynamic pointer parameter in
 kfuncs

On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen <
> > > jarkko@...nel.org> wrote:
> > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env
> > > > > *env, struct bpf_reg_state *reg,
> > > > > -                                  enum bpf_arg_type
> > > > > arg_type)
> > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > > > > struct bpf_reg_state *reg,
> > > > > +                           enum bpf_arg_type arg_type)
> > > > >  {
> > > > >       struct bpf_func_state *state = func(env, reg);
> > > > >       int spi = get_spi(reg->off);
> > > > > --
> > > > > 2.25.1
> > > > > 
> > > > 
> > > > Might be niticking but generally I'd consider splitting
> > > > exports as commits of their own.
> > > 
> > > -static bool
> > > +bool
> > > 
> > > into a separate commit?
> > > 
> > > I guess it makes sense for people whose salary depends on
> > > number of commits.
> > > We don't play these games.
> > 
> > What kind of argument is that anyway.
> 
> "Separate each *logical change* into a separate patch." [*]

The logical change, as per the patch subject, is allowing the
possibility of including eBPF dynamic pointers in a kfunc definition.
It requires to call an existing function that was already defined
elsewhere.

Maybe I'm wrong, but I don't see only exporting a function definition
to an include file as a logical change. To me, the changes in this
patch are clearly connected. Or even better, they tell why the function
definition has been exported, that would not appear if moving the
function definition is a standalone patch.

> 
> To add, generally any user space visible space should be an
> isolated patch.

As far as I understood, definitions visible to user space should be in
include/uapi.

> 
> Please, stop posting nonsense.

If I may, saying this does not encourage people to try to submit their
code. I feel it is a bit strong, and I kindly ask you to express your
opinion in a more gentle way.

Thanks

Roberto