From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oi1-x230.google.com (mail-oi1-x230.google.com [IPv6:2607:f8b0:4864:20::230]) by mail.toke.dk (Postfix) with ESMTPS id 9EAFA9C8E11 for ; Thu, 1 Dec 2022 01:32:57 +0100 (CET) Authentication-Results: mail.toke.dk; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20210112 header.b=BL2TqVq0 Received: by mail-oi1-x230.google.com with SMTP id s141so404031oie.10 for ; Wed, 30 Nov 2022 16:32:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=qf/BMEs9bgR379g3kVf+2jSNyHJ0315hhT9d1N7WzYU=; b=BL2TqVq0b1vLdrW8in8KaMpSdTqJPAQOimTS6oBexCA09t525q1m/RRfBvVwC0MRAO cpwlKoLiQpj9gvdq1W2COPSn/b220e4mB025docvuJdQJxqNlURnJqm0uaGh4l1tE4MV bs2jyH0KZiCP4r/ImYlnvT7LqM3Gv5/BL8mNESjT1w1cLpSK3S0KrupvD5H8dIiW9/92 9bI8FVcY6Z3bbMAlXfPdhgNJVJ1Qlco1yxEG7XMH7jIphSiWgXT8a1Y2lUfiY01aCMbn d41Xpay/JBoGUZB3ggjQJSFktgkFiSBMX/0pX4Kndg0i3+7e1GqcuUr/HRciEZQCuvir qvDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qf/BMEs9bgR379g3kVf+2jSNyHJ0315hhT9d1N7WzYU=; b=xMxiA5QG488KWIgYacS2B6/JhQkT4HO9zLi1hdptcqon1+A6kUm/odPAE515FY/0I1 cBzkOKbzL8lGkSlyvrbpfhG5WTQQ+F84rz5IrsArKRSygsVdgwFjuPQOyekxEBKHic38 i7m6PWyROIi31W8yiQWhrT+ATtL88nRPAl+VtLWCDXNzPsYoT4/tVJBE7s4euLUPMncQ /sAoI+1CIi4/JiUP6PdZXLI6HjO+N7wGxuoWJZWSnsITo+bjcZ+i1pEUH/gGk5eIK6ok KmjY2PoYTCEv4EXQwrLVpcPPQpDhlUyRZKkBiF0+KqwlUIIM21ohxD04TapPrX+Gdd3u 5x4w== X-Gm-Message-State: ANoB5pm4XpkUiWu7/eYz7yKTjfraFLZiVetYSZp7IfccT183zUJ+PuXq hl8SUTC9FD3XXnOiv1hSIN+x7u+URHMxYi91NOCTjg== X-Google-Smtp-Source: AA0mqf4HQxLdf1XvZmFeToHXrxubXKog8HmFrkwcba231TZQR8oI84iISFXiQSdzK1q8YzFrEINs6w3bC9i71atZCiM= X-Received: by 2002:a05:6808:a90:b0:35b:aa33:425a with SMTP id q16-20020a0568080a9000b0035baa33425amr9696615oij.181.1669854775273; Wed, 30 Nov 2022 16:32:55 -0800 (PST) MIME-Version: 1.0 References: <20221129193452.3448944-1-sdf@google.com> <8735a1zdrt.fsf@toke.dk> <87o7soxd1v.fsf@toke.dk> In-Reply-To: <87o7soxd1v.fsf@toke.dk> From: Stanislav Fomichev Date: Wed, 30 Nov 2022 16:32:44 -0800 Message-ID: To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: V46WUN75P6LZWGT4ID6RL4ZLUKDGIN3Q X-Message-ID-Hash: V46WUN75P6LZWGT4ID6RL4ZLUKDGIN3Q X-MailFrom: sdf@google.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, haoluo@google.com, jolsa@kernel.org, David Ahern , Jakub Kicinski , Willem de Bruijn , Jesper Dangaard Brouer , Anatoly Burakov , Alexander Lobakin , Magnus Karlsson , Maryam Tahhan , xdp-hints@xdp-project.net, netdev@vger.kernel.org X-Mailman-Version: 3.3.7 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next v3 00/11] xdp: hints via kfuncs List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, Nov 30, 2022 at 3:01 PM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > Stanislav Fomichev writes: > > > On Tue, Nov 29, 2022 at 12:50 PM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > >> > >> Stanislav Fomichev writes: > >> > >> > Please see the first patch in the series for the overall > >> > design and use-cases. > >> > > >> > Changes since v2: > >> > > >> > - Rework bpf_prog_aux->xdp_netdev refcnt (Martin) > >> > > >> > Switched to dropping the count early, after loading / verification= is > >> > done. At attach time, the pointer value is used only for comparing > >> > the actual netdev at attach vs netdev at load. > >> > >> So if we're not holding the netdev reference, we'll end up with a BPF > >> program with hard-coded CALL instructions calling into a module that > >> could potentially be unloaded while that BPF program is still alive, > >> right? > >> > >> I suppose that since we're checking that the attach iface is the same > >> that the program should not be able to run after the module is unloade= d, > >> but it still seems a bit iffy. And we should definitely block > >> BPF_PROG_RUN invocations of programs with a netdev set (but we should = do > >> that anyway). > > > > Ugh, good point about BPF_PROG_RUN, seems like it should be blocked > > regardless of the locking scheme though, right? > > Since our mlx4/mlx5 changes expect something after the xdp_buff, we > > can't use those per-netdev programs with our generic > > bpf_prog_test_run_xdp... > > Yup, I think we should just block it for now; maybe it can be enabled > later if it turns out to be useful (and we find a way to resolve the > kfuncs for this case). > > Also, speaking of things we need to disable, tail calls is another one. > And for freplace program attachment we need to add a check that the > target interfaces match as well. Agreed, thanks! > >> > (potentially can be a problem if the same slub slot is reused > >> > for another netdev later on?) > >> > >> Yeah, this would be bad as well, obviously. I guess this could happen? > > > > Not sure, that's why I'm raising it here to see what others think :-) > > Seems like this has to be actively exploited to happen? (and it's a > > privileged operation) > > > > Alternatively, we can go back to the original version where the prog > > holds the device. > > Matin mentioned in the previous version that if we were to hold a > > netdev refcnt, we'd have to drop it also from unregister_netdevice. > > Yeah; I guess we could keep a list of "bound" XDP programs in struct > net_device and clear each one on unregister? Also, bear in mind that the > "unregister" callback is also called when a netdev moves between > namespaces; which is probably not what we want in this case? > > > It feels like beyond that extra dev_put, we'd need to reset our > > aux->xdp_netdev and/or add some flag or something else to indicate > > that this bpf program is "orphaned" and can't be attached anywhere > > anymore (since the device is gone; netdev_run_todo should free the > > netdev it seems). > > You could add a flag, and change the check to: > > + if (new_prog->aux->xdp_has_netdev && > + new_prog->aux->xdp_netdev !=3D dev) { > + NL_SET_ERR_MSG(extack, "Cannot attach to a differ= ent target device"); > + return -EINVAL; > + } > > That way the check will always fail if xdp_netdev is reset to NULL > (while keeping the flag) on dereg? Something like that, yeah. I'll also take a closer look at offload.c as Martin points out. I should probably leverage it instead of trying to add more custom handling here.. > > That should address this potential issue with reusing the same addr > > for another netdev, but is a bit more complicated code-wise. > > Thoughts? > > I'd be in favour of adding this tracking; I worry that we'll end up with > some very subtle and hard-to-debug bugs if we somehow do end up > executing the wrong kfuncs... SG, will try to address soon!