Re: [PATCH v13 net-next 07/11] bpf: verifier (add ability to receive verification log)

[Daniel Borkmann]

On 09/17/2014 06:08 PM, Alexei Starovoitov wrote:
> On Tue, Sep 16, 2014 at 11:51 PM, Daniel Borkmann <dborkman@xxxxxxxxxx> wrote:
> > >    /* last field in 'union bpf_attr' used by this command */
> > > -#define        BPF_PROG_LOAD_LAST_FIELD license
> > > +#define        BPF_PROG_LOAD_LAST_FIELD log_buf
> >
> > I was looking to find a use case for this item, but couldn't find anything,
> > so
> > this seems to be dead code?
>
> See CHECK_ATTR() macro and comment next to it in patch #1
>
> > Was it, so that each time you extend an uapi structure like above that you
> > would
> > only access the structure up to BPF_PROG_LOAD_LAST_FIELD? That might not
> > work for
> > old binaries using this ABI running on newer kernels where there are
> > different
> > expectations of what BPF_PROG_LOAD_LAST_FIELD has been at the time of
> > compilation.
>
> exactly the opposite.
> CHECK_ATTR() is checking that all fields beyond last for given
> command are zero, so we can extend bpf_attr with new fields
> added after last.
> Transition from patch 4 to patch 7 and the hunk you quoted are
> demonstrating exactly that. Say, userspace was compiled
> with bpf_attr as defined in patch 4 and it populated fields all the way
> till 'license', and kernel is compiled with patch 7. Kernel does:
> union bpf_attr attr = {};
> /* copy attributes from user space, may be less than sizeof(bpf_attr) */
> copy_from_user(&attr, uattr, size)
> so newer fields (all the way till log_buf) stay zero and kernel
> behavior is the same as it was in patch 4.
> So older user space works as-is with newer kernel.

Ok, I see. Lets say, since the introduction of this syscall you have
added a couple of features and thus extended union bpf_attr where it
grew in size over time.

You built your shiny new binary on that uapi setting, and later on
decide to run it on an older kernel. What will happen is that in your
bpf syscall handler you will return with -EINVAL on that kernel right
away since the size of union bpf_attr is greater.

That would mean over time when new features will get added, applications
that want to make sure to run on _all_ kernels where the bpf syscall is
available have to make sure to either use the _initial_ version of
union bpf_attr in order to not get an -EINVAL, or worse they have
to probe though a syscall different versions of union bpf_attr if they
wish to make use of a particular feature until they do not get an -EINVAL
anymore.

I guess that might be problematic for an application developer that
wants to ship its application across different distributions usually
running different kernels. At least those people might then consider
holding a private copy of the _initial_ version of union bpf_attr in
their own source code, but it's not pleasant I guess.

I know you seem to have the constraint to run on NET-less systems, but
netlink could partially resolve that in the sense that it would allow
to at least load an eBPF program with initial feature set. Couldn't
there be some mechanism to make this interaction more pleasant? E.g.
in BPF extensions we can ask the kernel up to what extension it
supports and accordingly adapt to it up front. I know it's just a
/trivial/ example but have you thought about something on that kind for
the syscall?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/