On Fri, Sep 4, 2020 at 9:46 AM Leah Rumancik <leah.rumancik@xxxxxxxxx> wrote:
>
> 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?
"stable" in what sense? To run on different kernels ?
CO-RE already achieves that.
I think what I proposed above is "stable" enough based on the description
of what you wanted to achieve.
