-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_xdp_frags: check for privs early #316
test_xdp_frags: check for privs early #316
Conversation
As currently written, `test_xdp_frags` if run without necessary permissions will loudly print "this kernel does NOT support xdp frags", and then exit on the setrlimit(). The same thing happens if the argument list is invalid. Instead, verify all this before performing the frag support check, so as not to dismay users using this program to check for frag support. Signed-off-by: nick black <dankamongmen@gmail.com>
Hmm, interesting; I didn't anticipate someone would actually use this for that purpose. As for the second bit, can you be a bit more specific than "the formatting is all over the place"? 😅 |
as a pure user (i.e. didn't do any of the core development) of XDP and and i'm going to at some point need to know for my application whether the hardware stack on which i'm running supports frags/multibufs (of course, as there is no AF_XDP support yet, the answer for me would be 'none of them'). please don't interpret this as complaining! just letting you know where i'm coming from. i wrote this blog post last night you might find interesting:
using int main(int argc, char **argv)
{
^Istruct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
^Iint ifindex_bigmtu, ifindex_smallmtu, ret;
char *envval;
envval = secure_getenv("VERBOSE_TESTS");
^Isilence_libbpf_logging();
if (envval && envval[0] == '1')
verbose_libxdp_logging();
else
silence_libxdp_logging();
kern_compat = check_frags_compat();
^Iif (argc != 3)
usage(argv[0]);
^Iifindex_bigmtu = if_nametoindex(argv[1]);
^Iifindex_smallmtu = if_nametoindex(argv[2]);
^Iif (!ifindex_bigmtu || !ifindex_smallmtu) {
^I^Ifprintf(stderr, "Interface '%s' or '%s' not found.\n", argv[1], argv[2]);
usage(argv[0]);
^I}
^Iif (setrlimit(RLIMIT_MEMLOCK, &r)) {
^I^Ifprintf(stderr, "ERROR: setrlimit(RLIMIT_MEMLOCK) \"%s\"\n",
^I^I^Istrerror(errno));
^I^Iexit(EXIT_FAILURE);
^I}
ret = check_load_frags(kern_compat ? ifindex_bigmtu : 0, ifindex_smallmtu);
ret = check_load_nofrags_success(ifindex_smallmtu) || ret;
^Iif (kern_compat) {
^I^Iret = check_load_nofrags_fail(ifindex_bigmtu) || ret;
^I^Iret = check_load_frags_multi(ifindex_bigmtu) || ret;
ret = check_load_mix_big(ifindex_bigmtu) || ret;
^I}
^Iret = check_load_mix_small(ifindex_smallmtu) || ret;
^Ireturn ret;
} the |
nick black ***@***.***> writes:
> Hmm, interesting; I didn't anticipate someone would actually use this for that purpose.
as a pure user (i.e. didn't do any of the core development) of XDP and
`AF_XDP`, there are a lot of questions unanswered by the
documentation. in the hope of figuring some things out, i cast around
for any helpful tool =].
and i'm going to at some point need to know for my application whether
the hardware stack on which i'm running supports frags/multibufs (of
course, as there is no AF_XDP support yet, the answer for me would be
'none of them').
please don't interpret this as complaining! just letting you know
where i'm coming from.
It's quite OK to complain - usability of these things is severely
lacking in BPF/XDP land. I'm just intrigued that you actually found this
use, I'm not trying to disparage it, certainly. But as you say, ideally
we should have something more accessible.
There was recently a series merged in the upstream kernel that adds
explicit "feature flags" for XDP so userspace can query the kernel what
a certain netdevice supports. This is still fairly new on the kernel
side (I forget if it's 6.2 or 6.3 it landed in), but once it gets a bit
more widespread, the plan is to add support for this in xdp-tools so you
can do something like `xdp-loader features <dev>` and get a full list of
what that device supports (or doesn't support). So there is some hope
for a friendlier future, fortunately :)
i wrote this blog post last night you might find interesting:
https://nick-black.com/dankwiki/index.php/Extended_disquisitions_pertaining_to_eXpress_data_paths_(XDP)
Ah yes, very interesting. You are...not wrong.. about many of your
complaints. I opened #317 for that locking issue you mention; feel free
to open another one with specifics about those valgrind issues.
> As for the second bit, can you be a bit more specific than "the formatting is all over the place"? sweat_smile
using `cat -T`:
Ah, right. Yeah, those should really be tabs all of them; that's
probably my fault for not configuring my editor properly when writing
that code (Emacs has this weird issue with not always picking up the
tabs-only indentation thing, and I don't always notice). If you want to
fix it, doing so in a separate commit in the same PR should be fine...
|
As currently written,
test_xdp_frags
if run without necessary permissions will loudly print "this kernel does NOT support xdp frags", and then exit on thesetrlimit()
. The same thing happens if the argument list is invalid. Instead, verify all this before performing the frag support check, so as not to dismay users (like me) who try using this program to check for frag support.I didn't touch the formatting in this file so as to produce a minimal diff for review, but it's kinda all over the place.