Re: using verifier to ensure a BPF program uses certain metadata?

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Oct 16, 2017 at 09:38:44AM +0200, Johannes Berg wrote:
> Hi,
> 
> As we discussed in April already (it's really been that long...), I'd
> wanted to allow using BPF to filter wireless monitor frames, to enable
> new use cases and higher performance in monitoring. I have some code,
> at
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=bpf

bpf bits looks pretty straightforward.
attach looks fine too. I'm assuming there is some rtnl or other lock,
so multiple assigns cannot race?
It's missing query interface though.
Please add support to return prog_id.

> which implements parts of this. It's still missing the TX status path
> and perhaps associated metadata, but that part is easy.
> 
> The bigger "problem" is that we're going to be adding support for
> devices that have 802.11->Ethernet conversion already in hardware, and
> in that case the notion that the filter program will get an 802.11
> header to look at is no longer right.
> 
> Now, most likely for the actual in-service monitoring we'll actually
> have to reconstitute the 802.11 header on the fly (in pure monitoring
> where nothing else is active, we can just disable the conversion), but
> the filtering shouldn't really be reliant on that, since that's not the
> cheapest thing to do.
> 
> The obvious idea around this is to add a metadata field (just a bit
> really), something like "is_data_ethernet", saying that it was both a
> data frame and is already converted to have an Ethernet header.
> However, since these devices don't really exist yet for the vast
> majority of people, I'm a bit afraid that we'll find later a lot of
> code simply ignoring this field and looking at the "802.11" header,
> which is then broken if it encounters an Ethernet header instead.
> 
> Are there lies my question: If we added a new callback to
> bpf_verifier_ops (e.g. "post_verifier_check"), to be called after the
> normal verification, and also added a context argument to
> "is_valid_access" (*), we could easily track that this new metadata
> field is accessed, and reject programs that don't access it at all.
> 
> Now, I realize that people could trivially just work around this in
> their program if they wanted, but I think most will take the reminder
> and just implement
> 
>     if (ctx->is_data_ethernet)
>         return DROP_FRAME;
> 
> instead, since mostly data frames will not be very relevant to them.
> 
> What do you think?

sounds fine and considering new verifier ops after Jakub refactoring
a check that is_data_ethernet was accessed would fit nicely.
Without void** hack.