Netdev Archive on lore.kernel.org
 help / color / Atom feed
* Better ways to validate map via BTF?
@ 2019-11-28 16:08 Jesper Dangaard Brouer
  2019-11-29  5:27 ` Andrii Nakryiko
  0 siblings, 1 reply; 5+ messages in thread
From: Jesper Dangaard Brouer @ 2019-11-28 16:08 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: brouer, netdev, Toke Høiland-Jørgensen

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

Hi Andrii,

Is there are better way to validate that a userspace BPF-program uses
the correct map via BTF?

Below and in attached patch, I'm using bpf_obj_get_info_by_fd() to get
some map-info, and check info.value_size and info.max_entries match
what I expect.  What I really want, is to check that "map-value" have
same struct layout as:

 struct config {
	__u32 action;
	int ifindex;
	__u32 options;
 };

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


static void check_config_map_fd_info(int map_fd) {
	struct bpf_map_info info = { 0 };
	__u32 info_len = sizeof(info);
	__u32 exp_value_size = sizeof(struct config);
	__u32 exp_entries = 1;
	int err;

	/* BPF-info via bpf-syscall */
	err = bpf_obj_get_info_by_fd(map_fd, &info, &info_len);
	if (err) {
		fprintf(stderr, "ERR: %s() can't get info - %s\n",
			__func__,  strerror(errno));
		exit(EXIT_FAIL_BPF);
	}

	if (exp_value_size != info.value_size) {
		fprintf(stderr, "ERR: %s() "
			"Map value size(%d) mismatch expected size(%d)\n",
			__func__, info.value_size, exp_value_size);
		exit(EXIT_FAIL_BPF);
	}

	if (exp_entries != info.max_entries) {
		fprintf(stderr, "ERR: %s() "
			"Map max_entries(%d) mismatch expected entries(%d)\n",
			__func__, info.max_entries, exp_entries);
		exit(EXIT_FAIL_BPF);
	}
}


struct config {
	__u32 action;
	int ifindex;
	__u32 options;
};


[-- Attachment #2: 02-detect_map_mismatch --]
[-- Type: application/octet-stream, Size: 2423 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Better ways to validate map via BTF?
  2019-11-28 16:08 Better ways to validate map via BTF? Jesper Dangaard Brouer
@ 2019-11-29  5:27 ` Andrii Nakryiko
  2019-11-29  8:27   ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2019-11-29  5:27 UTC (permalink / raw)
  To: Jesper Dangaard Brouer; +Cc: netdev, Toke Høiland-Jørgensen

On Thu, Nov 28, 2019 at 8:08 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> Hi Andrii,


Hey, Jesper! Sorry for late reply, I'm on vacation for few days, so my
availability is irregular at best :)

>
> Is there are better way to validate that a userspace BPF-program uses
> the correct map via BTF?
>
> Below and in attached patch, I'm using bpf_obj_get_info_by_fd() to get
> some map-info, and check info.value_size and info.max_entries match
> what I expect.  What I really want, is to check that "map-value" have
> same struct layout as:
>
>  struct config {
>         __u32 action;
>         int ifindex;
>         __u32 options;
>  };

Well, there is no existing magical way to do this, but it is doable by
comparing BTFs of two maps. It's not too hard to compare all the
members of a struct, their names, sizes, types, etc (and do that
recursively, if necessary), but it's a bunch of code requiring due
diligence. Libbpf doesn't provide that in a ready-to-use form (it does
implement equivalence checks between two type graphs for dedup, but
it's quite coupled with and specific to BTF deduplication algorithm).
Keep in mind, when Toke implemented map pinning support in libbpf, we
decided to not check BTF for now, and just check key/value size,
flags, type, max_elements, etc.

>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>
>
> static void check_config_map_fd_info(int map_fd) {
>         struct bpf_map_info info = { 0 };
>         __u32 info_len = sizeof(info);
>         __u32 exp_value_size = sizeof(struct config);
>         __u32 exp_entries = 1;
>         int err;
>
>         /* BPF-info via bpf-syscall */
>         err = bpf_obj_get_info_by_fd(map_fd, &info, &info_len);
>         if (err) {
>                 fprintf(stderr, "ERR: %s() can't get info - %s\n",
>                         __func__,  strerror(errno));
>                 exit(EXIT_FAIL_BPF);
>         }
>
>         if (exp_value_size != info.value_size) {
>                 fprintf(stderr, "ERR: %s() "
>                         "Map value size(%d) mismatch expected size(%d)\n",
>                         __func__, info.value_size, exp_value_size);
>                 exit(EXIT_FAIL_BPF);
>         }
>
>         if (exp_entries != info.max_entries) {
>                 fprintf(stderr, "ERR: %s() "
>                         "Map max_entries(%d) mismatch expected entries(%d)\n",
>                         __func__, info.max_entries, exp_entries);
>                 exit(EXIT_FAIL_BPF);
>         }
> }
>
>
> struct config {
>         __u32 action;
>         int ifindex;
>         __u32 options;
> };
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Better ways to validate map via BTF?
  2019-11-29  5:27 ` Andrii Nakryiko
@ 2019-11-29  8:27   ` Toke Høiland-Jørgensen
  2019-12-02 20:03     ` Andrii Nakryiko
  0 siblings, 1 reply; 5+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-29  8:27 UTC (permalink / raw)
  To: Andrii Nakryiko, Jesper Dangaard Brouer; +Cc: netdev\

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Nov 28, 2019 at 8:08 AM Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
>>
>> Hi Andrii,
>
>
> Hey, Jesper! Sorry for late reply, I'm on vacation for few days, so my
> availability is irregular at best :)
>
>>
>> Is there are better way to validate that a userspace BPF-program uses
>> the correct map via BTF?
>>
>> Below and in attached patch, I'm using bpf_obj_get_info_by_fd() to get
>> some map-info, and check info.value_size and info.max_entries match
>> what I expect.  What I really want, is to check that "map-value" have
>> same struct layout as:
>>
>>  struct config {
>>         __u32 action;
>>         int ifindex;
>>         __u32 options;
>>  };
>
> Well, there is no existing magical way to do this, but it is doable by
> comparing BTFs of two maps. It's not too hard to compare all the
> members of a struct, their names, sizes, types, etc (and do that
> recursively, if necessary), but it's a bunch of code requiring due
> diligence. Libbpf doesn't provide that in a ready-to-use form (it does
> implement equivalence checks between two type graphs for dedup, but
> it's quite coupled with and specific to BTF deduplication algorithm).
> Keep in mind, when Toke implemented map pinning support in libbpf, we
> decided to not check BTF for now, and just check key/value size,
> flags, type, max_elements, etc.

Yeah. Probably a good idea to provide convenience functions for this in
libbpf (split out the existing code and make it more general?). Then we
can also use that for the test in the map pinning code :)

-Toke


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Better ways to validate map via BTF?
  2019-11-29  8:27   ` Toke Høiland-Jørgensen
@ 2019-12-02 20:03     ` Andrii Nakryiko
  2019-12-03 11:05       ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2019-12-02 20:03 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen; +Cc: Jesper Dangaard Brouer, netdev

On Fri, Nov 29, 2019 at 12:27 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Nov 28, 2019 at 8:08 AM Jesper Dangaard Brouer
> > <brouer@redhat.com> wrote:
> >>
> >> Hi Andrii,
> >
> >
> > Hey, Jesper! Sorry for late reply, I'm on vacation for few days, so my
> > availability is irregular at best :)
> >
> >>
> >> Is there are better way to validate that a userspace BPF-program uses
> >> the correct map via BTF?
> >>
> >> Below and in attached patch, I'm using bpf_obj_get_info_by_fd() to get
> >> some map-info, and check info.value_size and info.max_entries match
> >> what I expect.  What I really want, is to check that "map-value" have
> >> same struct layout as:
> >>
> >>  struct config {
> >>         __u32 action;
> >>         int ifindex;
> >>         __u32 options;
> >>  };
> >
> > Well, there is no existing magical way to do this, but it is doable by
> > comparing BTFs of two maps. It's not too hard to compare all the
> > members of a struct, their names, sizes, types, etc (and do that
> > recursively, if necessary), but it's a bunch of code requiring due
> > diligence. Libbpf doesn't provide that in a ready-to-use form (it does
> > implement equivalence checks between two type graphs for dedup, but
> > it's quite coupled with and specific to BTF deduplication algorithm).
> > Keep in mind, when Toke implemented map pinning support in libbpf, we
> > decided to not check BTF for now, and just check key/value size,
> > flags, type, max_elements, etc.
>
> Yeah. Probably a good idea to provide convenience functions for this in
> libbpf (split out the existing code and make it more general?). Then we
> can also use that for the test in the map pinning code :)

As I said, type graph equivalence for btf_dedup() is very specific to
dedup. It does deep (i.e., structs that are referenced by pointer only
also have to match exactly) and strict (const, volatile, typedefs, all
that matters **and** has to come in exactly the same order)
equivalence checks. In addition, it does forward declaration
resolution into concrete struct/union. So no, it can't be reused or
generalized.

It has to be a new code, but even then I'm hesitant to provide
something "generic", because it's again not clear what the right
semantics is for all the cases. E.g., should we ignore
const/volatile/restrict? Or, if some typedef is used, which ultimately
resolves to the same underlying type -- should we ignore such
differences? Also, should we follow and check types that are
referenced through pointers only? I think in different cases users
might be want to be strict or more lenient about such cases, which
suggests that we shouldn't have a generic API (at least yet, until we
see 2, 3, 4, real-life use cases). And there are more potential
differences in semantics without a clear answer of which one should be
used. So we can code it up for map pinning case (after having a
discussion of what two maps should be considered compatible), but I
don't think we should go all the way to exposing it as an API.


>
> -Toke
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Better ways to validate map via BTF?
  2019-12-02 20:03     ` Andrii Nakryiko
@ 2019-12-03 11:05       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 5+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-12-03 11:05 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: Jesper Dangaard Brouer, netdev\

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Nov 29, 2019 at 12:27 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Nov 28, 2019 at 8:08 AM Jesper Dangaard Brouer
>> > <brouer@redhat.com> wrote:
>> >>
>> >> Hi Andrii,
>> >
>> >
>> > Hey, Jesper! Sorry for late reply, I'm on vacation for few days, so my
>> > availability is irregular at best :)
>> >
>> >>
>> >> Is there are better way to validate that a userspace BPF-program uses
>> >> the correct map via BTF?
>> >>
>> >> Below and in attached patch, I'm using bpf_obj_get_info_by_fd() to get
>> >> some map-info, and check info.value_size and info.max_entries match
>> >> what I expect.  What I really want, is to check that "map-value" have
>> >> same struct layout as:
>> >>
>> >>  struct config {
>> >>         __u32 action;
>> >>         int ifindex;
>> >>         __u32 options;
>> >>  };
>> >
>> > Well, there is no existing magical way to do this, but it is doable by
>> > comparing BTFs of two maps. It's not too hard to compare all the
>> > members of a struct, their names, sizes, types, etc (and do that
>> > recursively, if necessary), but it's a bunch of code requiring due
>> > diligence. Libbpf doesn't provide that in a ready-to-use form (it does
>> > implement equivalence checks between two type graphs for dedup, but
>> > it's quite coupled with and specific to BTF deduplication algorithm).
>> > Keep in mind, when Toke implemented map pinning support in libbpf, we
>> > decided to not check BTF for now, and just check key/value size,
>> > flags, type, max_elements, etc.
>>
>> Yeah. Probably a good idea to provide convenience functions for this in
>> libbpf (split out the existing code and make it more general?). Then we
>> can also use that for the test in the map pinning code :)
>
> As I said, type graph equivalence for btf_dedup() is very specific to
> dedup. It does deep (i.e., structs that are referenced by pointer only
> also have to match exactly) and strict (const, volatile, typedefs, all
> that matters **and** has to come in exactly the same order)
> equivalence checks. In addition, it does forward declaration
> resolution into concrete struct/union. So no, it can't be reused or
> generalized.
>
> It has to be a new code, but even then I'm hesitant to provide
> something "generic", because it's again not clear what the right
> semantics is for all the cases. E.g., should we ignore
> const/volatile/restrict? Or, if some typedef is used, which ultimately
> resolves to the same underlying type -- should we ignore such
> differences? Also, should we follow and check types that are
> referenced through pointers only? I think in different cases users
> might be want to be strict or more lenient about such cases, which
> suggests that we shouldn't have a generic API (at least yet, until we
> see 2, 3, 4, real-life use cases). And there are more potential
> differences in semantics without a clear answer of which one should be
> used. So we can code it up for map pinning case (after having a
> discussion of what two maps should be considered compatible), but I
> don't think we should go all the way to exposing it as an API.

My immediate thought is that we'd want the strict interpretation by
default; at least for maps. My reasoning being that I expect most people
will just define a struct in a C file somewhere for their map contents,
and want to ensure that the map matches this, which would mean that any
changes to the struct definition should break the match.

I'll go read the dedup code, and try to base a comparison function for
maps on this; then we can discuss from there. I'm fine with keeping it
internal to begin with, but I worry that if we don't (eventually) expose
something as an API, people are just going to go the
reuse-via-copy-paste route instead. But sure, let's spend some time
collecting more experience with this before committing to an API.

-Toke


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 16:08 Better ways to validate map via BTF? Jesper Dangaard Brouer
2019-11-29  5:27 ` Andrii Nakryiko
2019-11-29  8:27   ` Toke Høiland-Jørgensen
2019-12-02 20:03     ` Andrii Nakryiko
2019-12-03 11:05       ` Toke Høiland-Jørgensen

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git