Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230613025226.3167956-1-imagedong@tencent.com>
 <20230613025226.3167956-3-imagedong@tencent.com> <ca490974-0c5c-cfe9-0c6f-3ead163e7a7b@meta.com>
 <7a82744f454944778f55c36e8445762f@AcuMS.aculab.com> <CADxym3bY5EcZhuJG=x5s7kH+BS93ySAyvV8yZ7yYoXf7HCsZVw@mail.gmail.com>
 <84050129b8ce4db9b4579be0fc022723@AcuMS.aculab.com>
In-Reply-To: <84050129b8ce4db9b4579be0fc022723@AcuMS.aculab.com>
From:   Menglong Dong <menglong8.dong@gmail.com>
Date:   Fri, 23 Jun 2023 21:08:49 +0800
Message-ID: <CADxym3akFCuMgQTF5kM1THUd8yofc4hVrACYGi3APwySftCtxA@mail.gmail.com>
Subject: Re: [PATCH bpf-next v5 2/3] bpf, x86: allow function arguments up to
 12 for TRACING
To:     David Laight <David.Laight@aculab.com>
Cc:     Yonghong Song <yhs@meta.com>,
        "alexei.starovoitov@gmail.com" <alexei.starovoitov@gmail.com>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "andrii@kernel.org" <andrii@kernel.org>,
        "martin.lau@linux.dev" <martin.lau@linux.dev>,
        "song@kernel.org" <song@kernel.org>, "yhs@fb.com" <yhs@fb.com>,
        "john.fastabend@gmail.com" <john.fastabend@gmail.com>,
        "kpsingh@kernel.org" <kpsingh@kernel.org>,
        "sdf@google.com" <sdf@google.com>,
        "haoluo@google.com" <haoluo@google.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "benbjiang@tencent.com" <benbjiang@tencent.com>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Menglong Dong <imagedong@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jun 22, 2023 at 10:19=E2=80=AFPM David Laight <David.Laight@aculab.=
com> wrote:
>
> ...
> > > Is that right for 86-64?
> > >
> > > IIRC arguments always take (at least) 64bits.
> > > For any 32bit argument (register or stack) the high bits are undefine=
d.
> > > (Maybe in kernel they are always zero?
> > > From 32bit userspace they are definitely random.)
> > >
> >
> > Hello,
> >
> > According to my testing, the compiler will always
> > pass the arguments on 8-byte size with "push" insn
> > if the count of the arguments that need to be passed
> > on stack more than 1 and the size of the argument
> > doesn't exceed 8-byte. In this case, there won't be
> > garbage. For example, the high 4-byte will be made 0
> > if the size of the argument is 4-byte, as the "push" insn
> > will copy the argument from regs or imm into stack
> > in 8-byte.
>
> You have to know whether a value is expected to be 4 or 8
> bytes - a negative 32bit value is zero extended so can't
> be treated as a 64bit value.
>
> That is even true for values passed in registers.
>
> There is also a common problem with values passed in registers
> to system calls by 32bit code (maybe bpf is tracing these).
> In this case the high 32 bits of the register are random.
> They don't get zerod in 32bit mode.
>
> > If the count of the arguments on-stack is 1 and its size
> > doesn't exceed 4-byte, some compiler, like clang, may
> > not use the "push" insn. Instead, it allocates 4 bytes in the
> > stack, and copies the arguments from regs or imm into
> > stack in 4-byte. This is the case we deal with here.
>
> If the compiler sometimes writes a 4 byte (or smaller) value
> to pre-allocated stack then it is always allowed to do that.
> So the high bytes of the stack slot that contains a 32bit
> argument might always be junk.
> The count of on-stack arguments isn't relevant.
>

Yes, the way we clean garbage values is not
relevant, which comes from assumption. However,
It should be ok with the BPF program? like what Yonghong
said.

> > I'm not sure if I understand you correctly. Do you mean
> > that there will be garbage values for 32bit args?
>
> I'm pretty sure that the function call ABI doesn't require the
> caller set the high bits of sub-64bit arguments.
> The fact that they are often written with a push instruction
> that zeros the high bytes isn't really relevant.
>
> > > I think the called code is also responsible form masking 8 and 16bit
> > > values (in reality char/short args and return values just add code
> > > bloat).
> > >
> > > A 128bit value is either passed in two registers or two stack
> > > slots. If the last register is skipped it will be used for the
> > > next argument.
> > >
> >
> > Yeah, this point is considered in save_args(). Once
> > this happen, the count of stack slots should more
> > then 1, and the arguments on-stack will be stored with
> > "push" insn in 8-byte. Therefore, there shouldn't be garbage
> > values in this case?
> >
> > Do I miss something?
>
> The register/stack for these two calls is the same:
>         foo(1, 2, 3, 4, 5, 6, (int128_t)7);
>         bar(1, 2, 3, 4, 5, (int128_t)7, 6);
>

It is ok, as we already consider such cases. For
the foo(), the order we copy args is:

reg1, reg2, reg3, reg4, reg5, reg6, stack1, stack2

and for the bar (), it is:

reg1, reg2, reg3, reg4, reg5, stack1,stack2, reg6

The order of the arguments in the array we passed
to the BPF program is ok.

Thanks!
Menglong Dong

>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1=
 1PT, UK
> Registration No: 1397386 (Wales)