| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 8 Aug 2019 12:52:11 -0700
From: Y Song <ys114321@...il.com>
To: Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
Cc: "Daniel T. Lee" <danieltimlee@...il.com>,
Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>,
netdev <netdev@...r.kernel.org>,
Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [v3,2/4] tools: bpftool: add net detach command to detach XDP on interface
On Wed, Aug 7, 2019 at 1:40 PM Maciej Fijalkowski
<maciejromanfijalkowski@...il.com> wrote:
>
> On Wed, 7 Aug 2019 13:12:17 -0700
> Y Song <ys114321@...il.com> wrote:
>
> > On Wed, Aug 7, 2019 at 11:30 AM Maciej Fijalkowski
> > <maciejromanfijalkowski@...il.com> wrote:
> > >
> > > On Wed, 7 Aug 2019 10:02:04 -0700
> > > Y Song <ys114321@...il.com> wrote:
> > >
> > > > On Tue, Aug 6, 2019 at 7:25 PM Daniel T. Lee <danieltimlee@...il.com> wrote:
> > > > >
> > > > > By this commit, using `bpftool net detach`, the attached XDP prog can
> > > > > be detached. Detaching the BPF prog will be done through libbpf
> > > > > 'bpf_set_link_xdp_fd' with the progfd set to -1.
> > > > >
> > > > > Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> > > > > ---
> > > > > tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> > > > > 1 file changed, 41 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > > > > index c05a3fac5cac..7be96acb08e0 100644
> > > > > --- a/tools/bpf/bpftool/net.c
> > > > > +++ b/tools/bpf/bpftool/net.c
> > > > > @@ -343,6 +343,43 @@ static int do_attach(int argc, char **argv)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int do_detach(int argc, char **argv)
> > > > > +{
> > > > > + enum net_attach_type attach_type;
> > > > > + int progfd, ifindex, err = 0;
> > > > > +
> > > > > + /* parse detach args */
> > > > > + if (!REQ_ARGS(3))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + attach_type = parse_attach_type(*argv);
> > > > > + if (attach_type == max_net_attach_type) {
> > > > > + p_err("invalid net attach/detach type");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + NEXT_ARG();
> > > > > + ifindex = net_parse_dev(&argc, &argv);
> > > > > + if (ifindex < 1)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + /* detach xdp prog */
> > > > > + progfd = -1;
> > > > > + if (is_prefix("xdp", attach_type_strings[attach_type]))
> > > > > + err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);
> > > >
> > > > I found an issue here. This is probably related to do_attach_detach_xdp.
> > > >
> > > > -bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
> > > > -bash-4.4$ sudo ./bpftool net
> > > > xdp:
> > > > v1(4) driver id 1172
> > > >
> > > > tc:
> > > > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > > > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > > > eth0(2) clsact/egress fbflow_egress id 28
> > > > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> > > >
> > > > flow_dissector:
> > > >
> > > > -bash-4.4$ sudo ./bpftool net detach x dev v2
> > >
> > > Shouldn't this be v1 as dev?
> >
> > I am testing a scenario where with wrong devname
> > we did not return an error.
>
> Ah ok. In this scenario if driver has a native xdp support we would be invoking
> its ndo_bpf even if there's no prog currently attached and it wouldn't return
> error value.
>
> Looking at dev_xdp_uninstall, setting driver's prog to NULL is being done only
> when prog is attached. Maybe we should consider querying the driver in
> dev_change_xdp_fd regardless of passed fd value? E.g. don't query only when
> prog >= 0.
>
> I don't recall whether this was brought up previously.
Thanks for explanation. I think return an error is better in
such error cases. Otherwise, people mistakenly write wrong
device name and they may think xdp is detached and it is
actually not.
But this probably should not prevent
this patch as it is more like a kernel issue.
>
> CCing Jakub so we have one thread.
>
> Maciej
>
> >
> > Yes, if dev "v1", it works as expected.
> >
> > >
> > > > -bash-4.4$ sudo ./bpftool net
> > > > xdp:
> > > > v1(4) driver id 1172
> > > >
> > > > tc:
> > > > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > > > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > > > eth0(2) clsact/egress fbflow_egress id 28
> > > > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> > > >
> > > > flow_dissector:
> > > >
> > > > -bash-4.4$
> > > >
> > > > Basically detaching may fail due to wrong dev name or wrong type, etc.
> > > > But the tool did not return an error. Is this expected?
> > > > This may be related to this funciton "bpf_set_link_xdp_fd()".
> > > > So this patch itself should be okay.
> > > >
> > > > > +
> > > > > + if (err < 0) {
> > > > > + p_err("interface %s detach failed",
> > > > > + attach_type_strings[attach_type]);
> > > > > + return err;
> > > > > + }
> > > > > +
> > > > > + if (json_output)
> > > > > + jsonw_null(json_wtr);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static int do_show(int argc, char **argv)
> > > > > {
> > > > > struct bpf_attach_info attach_info = {};
> > > > > @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
> > > > > fprintf(stderr,
> > > > > "Usage: %s %s { show | list } [dev <devname>]\n"
> > > > > " %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> > > > > + " %s %s detach ATTACH_TYPE dev <devname>\n"
> > > > > " %s %s help\n"
> > > > > "\n"
> > > > > " " HELP_SPEC_PROGRAM "\n"
> > > > > @@ -429,7 +467,8 @@ static int do_help(int argc, char **argv)
> > > > > " to dump program attachments. For program types\n"
> > > > > " sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
> > > > > " consult iproute2.\n",
> > > > > - bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
> > > > > + bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
> > > > > + bin_name, argv[-2]);
> > > > >
> > > > > return 0;
> > > > > }
> > > > > @@ -438,6 +477,7 @@ static const struct cmd cmds[] = {
> > > > > { "show", do_show },
> > > > > { "list", do_show },
> > > > > { "attach", do_attach },
> > > > > + { "detach", do_detach },
> > > > > { "help", do_help },
> > > > > { 0 }
> > > > > };
> > > > > --
> > > > > 2.20.1
> > > > >
> > >
>
Powered by blists - more mailing lists