On Mon, Aug 17, 2020 at 09:32:07AM -0700, Alexei Starovoitov wrote:
> On Mon, Aug 17, 2020 at 10:18:47PM +0800, Bob Liu wrote:
> > > +
> > > +/* allows IO by default if no programs attached */
> > > +int io_filter_bpf_run(struct bio *bio)
> > > +{
> > > + struct bpf_io_request io_req = {
> > > + .sector_start = bio->bi_iter.bi_sector,
> > > + .sector_cnt = bio_sectors(bio),
> > > + .opf = bio->bi_opf,
> > > + };
> > > +
> > > + return BPF_PROG_RUN_ARRAY_CHECK(bio->bi_disk->progs, &io_req, BPF_PROG_RUN);
> >
> >
> > I think pass "struct bpf_io_request" is not enough, since we may want to do the filter based on
> > some special patterns against the io data.
> >
> > I used to pass "page_to_virt(bio->bi_io_vec->bv_page)" into ebpf program..
>
> Bob,
>
> Just like other bpf uapi structs the bpf_io_request is extensible and
> such pointer can be added later, but I have a different question.
>
> Leah,
>
> Do you really need the arguments to be stable?
> If so 'opf' above is not enough.
> sector_start, sector_cnt are clean from uapi pov,
> but 'opf' exposes kernel internals.
> The patch 2 is doing:
> +int protect_gpt(struct bpf_io_request *io_req)
> +{
> + /* within GPT and not a read operation */
> + if (io_req->sector_start < GPT_SECTORS && (io_req->opf & REQ_OP_MASK) != REQ_OP_READ)
> + return IO_BLOCK;
>
> The way ops are encoded changed quite a bit over the kernel releases.
> First it was REQ_WRITE, then REQ_OP_SHIFT, now REQ_OP_MASK.
> From kernel pov it would be simpler if bpf side didn't impose stability
> requriment on the program arguments. Then the kernel will be free to change
> REG_OP_READ into something else. The progs would break, of course, and would
> have to be adjusted. That's what we've been doing with tools like biosnoop.
> If you're ok with unstable arguments then you wouldn't need to introduce
> new prog type and this patch set.
> You can do this filtering already with should_fail_bio().
> bpf prog can attach to should_fail_bio() and walk all bio arguments
> in unstable way.
> Instead of:
> + if (io_req->sector_start < GPT_SECTORS && (io_req->opf & REQ_OP_MASK) != REQ_OP_READ)
> you'll write:
> if (bio->bi_iter.bi_sector < GPT_SECTORS && (bio->bi_opf & REQ_OP_MASK) != REQ_OP_READ)
> It will also work on different kernels because libbpf can adjust field offsets and
> check for type matching via CO-RE facility.
> Will that work for you?
Alexei,
I need the arguments to be stable. What would be the best way to go
about this? Pulling selected information from the opf field and defining
my own constants?
Thanks,
Leah
