On Thu, Sep 08, 2022 at 09:24:19AM -0700, Vincent Li wrote:
> On Wed, Sep 7, 2022 at 8:51 PM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
> >
> > On Wed, Sep 07, 2022 at 07:40:55PM -0700, Vincent Li wrote:
> > > On Wed, Sep 7, 2022 at 7:35 PM Vincent Li <vincent.mc.li@xxxxxxxxx> wrote:
> > > > Hi,
> > > >
> > > > I see some verifier log examples with error like:
> > > >
> > > > R4 invalid mem access 'inv'
> > > >
> > > > It looks like invalid mem access errors occur in two places below,
> > > > does it make sense to make the error message slightly different so for
> > > > new eBPF programmers like me to tell the first invalid mem access is
> > > > maybe the memory is NULL? and the second invalid mem access is because
> > > > the register type does not match any valid memory pointer? or this
> > > > won't help identifying problems and don't bother ?
> > > >
> > > > 4772 } else if (base_type(reg->type) == PTR_TO_MEM) {
> > > > 4773 bool rdonly_mem = type_is_rdonly_mem(reg->type);
> > > > 4774
> > > > 4775 if (type_may_be_null(reg->type)) {
> > > > 4776 verbose(env, "R%d invalid mem access
> > > > '%s'\n", regno,
> > > > 4777 reg_type_str(env, reg->type));
> >
> > While the error you're seeing is coming from the bottom case (more on that
> > below), I agree hinting the user that a null check is missing may be
> > helpful.
> >
> right, I think the reg_type_str will output the 'nul' string in this
> case if I read the code correct.
The reg_type_str() output should be "mem_or_null" in this case since base_type(reg->type) == PTR_TO_MEM and type_may_be_null(reg->type) == true
static const char *reg_type_str(struct bpf_verifier_env *env,
enum bpf_reg_type type)
{
static const char * const str[] = {
...
[PTR_TO_MEM] = "mem",
[PTR_TO_BUF] = "buf",
[PTR_TO_FUNC] = "func",
[PTR_TO_MAP_KEY] = "map_key",
};
if (type & PTR_MAYBE_NULL) {
if (base_type(type) == PTR_TO_BTF_ID)
strncpy(postfix, "or_null_", 16);
else
strncpy(postfix, "_or_null", 16);
}
...
snprintf(env->type_str_buf, TYPE_STR_BUF_LEN, "%s%s%s",
prefix, str[base_type(type)], postfix);
return env->type_str_buf;
}
> > > > 4778 return -EACCES;
> > > > 4779 }
> > > >
> > > > and
> > > >
> > > > 4924 } else {
> > > > 4925 verbose(env, "R%d invalid mem access '%s'\n", regno,
> > > > 4926 reg_type_str(env, reg->type));
> > > > 4927 return -EACCES;
> > > > 4928 }
> > >
> > > sorry I should read the code more carefully, I guess the "inv" already
> > > says it is invalid memory access, not NULL, right?
> >
> > The "inv" actually means that the type of R4 is scalar. IIUC "inv" stands
> > for invariant, which is a term used in static analysis.
> >
> > Since v5.18 (commit 7df5072cc05f "bpf: Small BPF verifier log improvements")
> > the verifier will say "scalar" instead.
>
> Thanks for the clarification :)
Your welcome :)
