On Wed, Nov 10, 2021 at 2:05 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Wed, Nov 10, 2021 at 11:55 AM Hao Luo <haoluo@xxxxxxxxxx> wrote: > > > > On Tue, Nov 9, 2021 at 8:43 PM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > On Mon, Nov 8, 2021 at 4:31 PM Hao Luo <haoluo@xxxxxxxxxx> wrote: > > > > > > > > There are currently two ways to modify a kernel memory in bpf programs: > > > > 1. declare a ksym of scalar type and directly modify its memory. > > > > 2. Pass a RDONLY_BUF into a helper function which will override > > > > its arguments. For example, bpf_d_path, bpf_snprintf. > > > > > > > > This patchset fixes these two problem. For the first, we introduce a > > > > new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids > > > > writing. For the second, we introduce a new arg type ARG_CONST_PTR_TO_MEM > > > > to differentiate the arg types that only read the memory from those > > > > that may write the memory. The previous ARG_PTR_TO_MEM is now only > > > > compatible with writable memories. If a helper doesn't write into its > > > > argument, it can use ARG_CONST_PTR_TO_MEM, which is also compatible > > > > with read-only memories. > > > > > > > > In v2, Andrii suggested using the name "ARG_PTR_TO_RDONLY_MEM", but I > > > > find it is sort of misleading. Because the new arg_type is compatible > > > > with both write and read-only memory. So I chose ARG_CONST_PTR_TO_MEM > > > > instead. > > > > > > I find ARG_CONST_PTR_TO_MEM misleading. It's the difference between > > > `char * const` (const pointer to mutable memory) vs `const char *` > > > (pointer to an immutable memory). We need the latter semantics, and > > > that *is* PTR_TO_RDONLY_MEM in BPF verifier terms. > > > > > > > Ah, I am aware of the semantic difference between 'char * const' and > > 'const char *', but your explanation in the bracket helps me see your > > point better. It does seem PTR_TO_RDONLY_MEM matches the semantics > > now. Let me fix and send an update. > > I thought earlier we agreed that flag approach is prefered. > Looks like OR_NULL discussion is progressing nicely and > IS_RDONLY or IS_RDWR flags will just fit right in. > What's the reason to go with this approach ? > It seems it will make the refactoring more tedious later. I felt this patch series has been holding for too long and is now mostly ready. In contrast, I wasn't sure how many iterations are needed for the flag patch and kind of worried it may further delay this patch, so I developed these two patches independently. I can certainly merge them into one if that's preferred.
