[PATCH net] nfp: bpf: Add an MTU check before offloading BPF

[PATCH net] nfp: bpf: Add an MTU check before offloading BPF

[Simon Horman]

From: Yu Xiao <yu.xiao@corigine.com>

There is a bug during xdpoffloading. When MTU is bigger than the
max MTU of BFP (1888), it can still be added xdpoffloading.

Therefore, add an MTU check to ensure that xdpoffloading cannot be
loaded when MTU is larger than a max MTU of 1888.

Fixes: 6d6770755f05 ("nfp: add support for offload of XDP programs")
Signed-off-by: Yu Xiao <yu.xiao@corigine.com>
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index 11c83a99b014..105142437fb4 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -34,11 +34,25 @@ static bool nfp_net_ebpf_capable(struct nfp_net *nn)
 #endif
 }
 
+static inline unsigned int
+nfp_bpf_get_bpf_max_mtu(struct nfp_net *nn)
+{
+	return nn_readb(nn, NFP_NET_CFG_BPF_INL_MTU) * 64 - 32;
+}
+
 static int
 nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
 		    struct bpf_prog *prog, struct netlink_ext_ack *extack)
 {
 	bool running, xdp_running;
+	unsigned int max_mtu;
+
+	max_mtu = nfp_bpf_get_bpf_max_mtu(nn);
+	if (nn->dp.mtu > max_mtu) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "port MTU over max MTU of BPF offloading not supported");
+		return -EINVAL;
+	}
 
 	if (!nfp_net_ebpf_capable(nn))
 		return -EINVAL;
@@ -187,7 +201,7 @@ nfp_bpf_check_mtu(struct nfp_app *app, struct net_device *netdev, int new_mtu)
 	if (~nn->dp.ctrl & NFP_NET_CFG_CTRL_BPF)
 		return 0;
 
-	max_mtu = nn_readb(nn, NFP_NET_CFG_BPF_INL_MTU) * 64 - 32;
+	max_mtu = nfp_bpf_get_bpf_max_mtu(nn);
 	if (new_mtu > max_mtu) {
 		nn_info(nn, "BPF offload active, MTU over %u not supported\n",
 			max_mtu);
-- 
2.20.1

Re: [PATCH net] nfp: bpf: Add an MTU check before offloading BPF

[Jakub Kicinski]

On Wed, 29 Sep 2021 17:24:21 +0200 Simon Horman wrote:
> From: Yu Xiao <yu.xiao@corigine.com>
> 
> There is a bug during xdpoffloading. When MTU is bigger than the
> max MTU of BFP (1888), it can still be added xdpoffloading.
> 
> Therefore, add an MTU check to ensure that xdpoffloading cannot be
> loaded when MTU is larger than a max MTU of 1888.

There is a check in nfp_net_bpf_load(). TC or XDP, doesn't matter,
we can't offload either with large MTU since the FW helper (used to be) 
able to only access CTM. So the check is on the generic path, adding
an XDP-specific check seems wrong.

Re: [PATCH net] nfp: bpf: Add an MTU check before offloading BPF

[Niklas Söderlund]

Hello Jackub,

Thanks for your feedback.

On 2021-09-29 11:47:48 -0700, Jakub Kicinski wrote:
> On Wed, 29 Sep 2021 17:24:21 +0200 Simon Horman wrote:
> > From: Yu Xiao <yu.xiao@corigine.com>
> > 
> > There is a bug during xdpoffloading. When MTU is bigger than the
> > max MTU of BFP (1888), it can still be added xdpoffloading.
> > 
> > Therefore, add an MTU check to ensure that xdpoffloading cannot be
> > loaded when MTU is larger than a max MTU of 1888.
> 
> There is a check in nfp_net_bpf_load(). TC or XDP, doesn't matter,
> we can't offload either with large MTU since the FW helper (used to be) 
> able to only access CTM. So the check is on the generic path, adding
> an XDP-specific check seems wrong.

I understand your point and it make sens. The check in 
nfp_net_bpf_load() in the generic path do indeed check for this, but in 
a slightly different way. It verifies that the BPF program don't access 
any data that is not in CMT.

The original problem this patch tried to address was to align the 
behavior that the MTU is verified differently when the BPF program is 
loaded and when the MTU is changed once the program is loaded.

Without this patch we had the following behavior,

    # ip link set ens5np0 mtu 9000
    # ip link set dev ens5np0 xdpoffload obj bpf_prog.o sec testcase
    # ip link show dev ens5np0
    11: ens5np0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 xdpoffload qdisc mq state UP mode DEFAULT group default qlen 1000
	link/ether 00:15:4d:13:61:91 brd ff:ff:ff:ff:ff:ff
	prog/xdp id 48 tag 57cd311f2e27366b jited
    # ip link set ens5np0 mtu 1500
    # ip link set ens5np0 mtu 9000
    RTNETLINK answers: Device or resource busy
    # ip link set ens5np0 mtu 1888
    # ip link show dev ens5np0
    11: ens5np0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1888 xdpoffload qdisc mq state UP mode DEFAULT group default qlen 1000
        link/ether 00:15:4d:13:61:91 brd ff:ff:ff:ff:ff:ff
        prog/xdp id 48 tag 57cd311f2e27366b jited 

When the MTU is changed after the program is offloaded the check in 
nfp_bpf_check_mtu() is consulted and as it checks the MTU differently 
and fails the change. Maybe we should align this the other way around 
and update the check in nfp_bpf_check_mtu() to match the one in 
nfp_net_bpf_load()?

On a side note the check in nfp_net_bpf_load() allows for BPF programs 
to be offloaded that do access data beyond the CMT size limit provided 
the MTU is set below the CMT threshold value. There should be no real 
harm in this as the verifier forces bounds check so with a MTU small 
enough it should never happen. But maybe we should add a check for this 
too to prevent such a program to be loaded in the first place.

Thanks again for your input.

-- 
Regards,
Niklas Söderlund

Re: [PATCH net] nfp: bpf: Add an MTU check before offloading BPF

[Jakub Kicinski]

On Thu, 30 Sep 2021 16:46:34 +0200 Niklas Söderlund wrote:
> When the MTU is changed after the program is offloaded the check in 
> nfp_bpf_check_mtu() is consulted and as it checks the MTU differently 
> and fails the change. Maybe we should align this the other way around 
> and update the check in nfp_bpf_check_mtu() to match the one in 
> nfp_net_bpf_load()?

That sounds reasonable. Although I don't remember how reliable the
max_pkt_offset logic is in practice (whether it's actually capable 
of finding the max offset for realistic programs or it's mostly going
to be set to MAX).

> On a side note the check in nfp_net_bpf_load() allows for BPF programs 
> to be offloaded that do access data beyond the CMT size limit provided 
> the MTU is set below the CMT threshold value.

Right, because of variable length offsets verifier will not be able to
estimate max_pkt_offset.

> There should be no real harm in this as the verifier forces bounds
> check so with a MTU small enough it should never happen. But maybe we
> should add a check for this too to prevent such a program to be
> loaded in the first place.

Re: [PATCH net] nfp: bpf: Add an MTU check before offloading BPF

[Niklas Söderlund]

On 2021-09-30 07:59:59 -0700, Jakub Kicinski wrote:
> On Thu, 30 Sep 2021 16:46:34 +0200 Niklas Söderlund wrote:
> > When the MTU is changed after the program is offloaded the check in 
> > nfp_bpf_check_mtu() is consulted and as it checks the MTU differently 
> > and fails the change. Maybe we should align this the other way around 
> > and update the check in nfp_bpf_check_mtu() to match the one in 
> > nfp_net_bpf_load()?
> 
> That sounds reasonable. Although I don't remember how reliable the
> max_pkt_offset logic is in practice (whether it's actually capable 
> of finding the max offset for realistic programs or it's mostly going
> to be set to MAX).
> 
> > On a side note the check in nfp_net_bpf_load() allows for BPF programs 
> > to be offloaded that do access data beyond the CMT size limit provided 
> > the MTU is set below the CMT threshold value.
> 
> Right, because of variable length offsets verifier will not be able to
> estimate max_pkt_offset.

Thanks, this made the design click for me.

> 
> > There should be no real harm in this as the verifier forces bounds
> > check so with a MTU small enough it should never happen. But maybe we
> > should add a check for this too to prevent such a program to be
> > loaded in the first place.

-- 
Regards,
Niklas Söderlund