[PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Stanislav Fomichev]

Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
packets") exposed the fact that bpf_clone_redirect is capable of
returning raw NET_XMIT_XXX return codes.

This is in the conflict with its UAPI doc which says the following:
"0 on success, or a negative error in case of failure."

Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
as -ENOBUFS instead of 1.

Note, this is technically breaking existing UAPI where we used to
return 1 and now will do -ENOBUFS. The alternative is to
document that bpf_clone_redirect can return 1 for DROP and 2 for CN.

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/core/filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index a094694899c9..9e297931b02f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
 	ret = dev_queue_xmit(skb);
 	dev_xmit_recursion_dec();
 
+	if (ret > 0)
+		ret = net_xmit_errno(ret);
+
 	return ret;
 }
 
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH bpf-next 2/2] selftests/bpf: update bpf_clone_redirect expected return code

[Stanislav Fomichev]

Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
packets") started propagating proper NET_XMIT_DROP error into
the caller which means it's now possible to get -ENOBUFS when
calling bpf_clone_redirect() in this particular test. Update the
test to reflect that.

Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/prog_tests/empty_skb.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/empty_skb.c b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
index 3b77d8a422db..b9f5cb312033 100644
--- a/tools/testing/selftests/bpf/prog_tests/empty_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
@@ -24,6 +24,7 @@ void test_empty_skb(void)
 		int *ifindex;
 		int err;
 		int ret;
+		int lwt_egress_ret; /* expected retval at lwt/egress */
 		bool success_on_tc;
 	} tests[] = {
 		/* Empty packets are always rejected. */
@@ -57,6 +58,7 @@ void test_empty_skb(void)
 			.data_size_in = sizeof(eth_hlen),
 			.ifindex = &veth_ifindex,
 			.ret = -ERANGE,
+			.lwt_egress_ret = -ERANGE,
 			.success_on_tc = true,
 		},
 		{
@@ -70,6 +72,7 @@ void test_empty_skb(void)
 			.data_size_in = sizeof(eth_hlen),
 			.ifindex = &ipip_ifindex,
 			.ret = -ERANGE,
+			.lwt_egress_ret = -ERANGE,
 		},
 
 		/* ETH_HLEN+1-sized packet should be redirected. */
@@ -79,6 +82,7 @@ void test_empty_skb(void)
 			.data_in = eth_hlen_pp,
 			.data_size_in = sizeof(eth_hlen_pp),
 			.ifindex = &veth_ifindex,
+			.lwt_egress_ret = -ENOBUFS,
 		},
 		{
 			.msg = "ipip ETH_HLEN+1 packet ingress",
@@ -108,8 +112,12 @@ void test_empty_skb(void)
 
 	for (i = 0; i < ARRAY_SIZE(tests); i++) {
 		bpf_object__for_each_program(prog, bpf_obj->obj) {
-			char buf[128];
+			bool at_egress = strstr(bpf_program__name(prog), "egress") != NULL;
 			bool at_tc = !strncmp(bpf_program__section_name(prog), "tc", 2);
+			int expected_ret;
+			char buf[128];
+
+			expected_ret = at_egress && !at_tc ? tests[i].lwt_egress_ret : tests[i].ret;
 
 			tattr.data_in = tests[i].data_in;
 			tattr.data_size_in = tests[i].data_size_in;
@@ -128,7 +136,7 @@ void test_empty_skb(void)
 			if (at_tc && tests[i].success_on_tc)
 				ASSERT_GE(bpf_obj->bss->ret, 0, buf);
 			else
-				ASSERT_EQ(bpf_obj->bss->ret, tests[i].ret, buf);
+				ASSERT_EQ(bpf_obj->bss->ret, expected_ret, buf);
 		}
 	}
 
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Martin KaFai Lau]

On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
> Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
> packets") exposed the fact that bpf_clone_redirect is capable of
> returning raw NET_XMIT_XXX return codes.
> 
> This is in the conflict with its UAPI doc which says the following:
> "0 on success, or a negative error in case of failure."
> 
> Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
> net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
> as -ENOBUFS instead of 1.
> 
> Note, this is technically breaking existing UAPI where we used to
> return 1 and now will do -ENOBUFS. The alternative is to
> document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> 
> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>   net/core/filter.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index a094694899c9..9e297931b02f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
>   	ret = dev_queue_xmit(skb);
>   	dev_xmit_recursion_dec();
>   
> +	if (ret > 0)
> +		ret = net_xmit_errno(ret);

I think it is better to have bpf_clone_redirect returning -ENOBUFS instead of 
leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the uapi/bpf.h also 
mentions

  *      Return
  *              0 on success, or a negative error in case of failure.

If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for 
__bpf_rx_skb? and should net_xmit_errno() only be done for bpf_clone_redirect()? 
  __bpf_{tx,rx}_skb is also used by skb_do_redirect() which also calls 
__bpf_redirect_neigh() that returns NET_XMIT_xxx but no caller seems to care the 
NET_XMIT_xxx value now.

Daniel should know more here. I would wait for Daniel to comment.

~~~~

For the selftest, may be another option is to use a 28 bytes data_in for the lwt 
program redirecting to veth? 14 bytes used by bpf_prog_test_run_skb and leave 14 
bytes for veth_xmit. It seems the original intention of the "veth ETH_HLEN+1 
packet ingress" test is expecting it to succeed also.

> +
>   	return ret;
>   }
>   

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Stanislav Fomichev]

On 09/09, Martin KaFai Lau wrote:
> On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
> > Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
> > packets") exposed the fact that bpf_clone_redirect is capable of
> > returning raw NET_XMIT_XXX return codes.
> > 
> > This is in the conflict with its UAPI doc which says the following:
> > "0 on success, or a negative error in case of failure."
> > 
> > Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
> > net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
> > as -ENOBUFS instead of 1.
> > 
> > Note, this is technically breaking existing UAPI where we used to
> > return 1 and now will do -ENOBUFS. The alternative is to
> > document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> > 
> > Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >   net/core/filter.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index a094694899c9..9e297931b02f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
> >   	ret = dev_queue_xmit(skb);
> >   	dev_xmit_recursion_dec();
> > +	if (ret > 0)
> > +		ret = net_xmit_errno(ret);
> 
> I think it is better to have bpf_clone_redirect returning -ENOBUFS instead
> of leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the
> uapi/bpf.h also mentions
> 
>  *      Return
>  *              0 on success, or a negative error in case of failure.
> 
> If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for
> __bpf_rx_skb? and should net_xmit_errno() only be done for
> bpf_clone_redirect()?  __bpf_{tx,rx}_skb is also used by skb_do_redirect()
> which also calls __bpf_redirect_neigh() that returns NET_XMIT_xxx but no
> caller seems to care the NET_XMIT_xxx value now.

__bpf_rx_skb seems to only add to backlog and doesn't seem to return any
of the NET_XMIT_xxx. But I might be wrong and haven't looked too deep
into that.

> Daniel should know more here. I would wait for Daniel to comment.

Ack, sure!

> ~~~~
> 
> For the selftest, may be another option is to use a 28 bytes data_in for the
> lwt program redirecting to veth? 14 bytes used by bpf_prog_test_run_skb and
> leave 14 bytes for veth_xmit. It seems the original intention of the "veth
> ETH_HLEN+1 packet ingress" test is expecting it to succeed also.

IIUC, you're suggesting to pass full ipv4 or ipv6 packet for veth tests
to make them actually succeed with the forwarding, right?

Sure, I can do that. But let's keep this entry with the -NOBUFS as well?
Just for the sake of ensuring that we don't export NET_XMIT_xxx from
uapi.

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Daniel Borkmann]

On 9/11/23 7:11 PM, Stanislav Fomichev wrote:
> On 09/09, Martin KaFai Lau wrote:
>> On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
>>> Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
>>> packets") exposed the fact that bpf_clone_redirect is capable of
>>> returning raw NET_XMIT_XXX return codes.
>>>
>>> This is in the conflict with its UAPI doc which says the following:
>>> "0 on success, or a negative error in case of failure."
>>>
>>> Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
>>> net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
>>> as -ENOBUFS instead of 1.
>>>
>>> Note, this is technically breaking existing UAPI where we used to
>>> return 1 and now will do -ENOBUFS. The alternative is to
>>> document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
>>>
>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>> ---
>>>    net/core/filter.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index a094694899c9..9e297931b02f 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
>>>    	ret = dev_queue_xmit(skb);
>>>    	dev_xmit_recursion_dec();
>>> +	if (ret > 0)
>>> +		ret = net_xmit_errno(ret);
>>
>> I think it is better to have bpf_clone_redirect returning -ENOBUFS instead
>> of leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the
>> uapi/bpf.h also mentions
>>
>>   *      Return
>>   *              0 on success, or a negative error in case of failure.
>>
>> If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for
>> __bpf_rx_skb? and should net_xmit_errno() only be done for
>> bpf_clone_redirect()?  __bpf_{tx,rx}_skb is also used by skb_do_redirect()
>> which also calls __bpf_redirect_neigh() that returns NET_XMIT_xxx but no
>> caller seems to care the NET_XMIT_xxx value now.
> 
> __bpf_rx_skb seems to only add to backlog and doesn't seem to return any
> of the NET_XMIT_xxx. But I might be wrong and haven't looked too deep
> into that.
> 
>> Daniel should know more here. I would wait for Daniel to comment.
> 
> Ack, sure!

I think my preference would be to just document it in the helper UAPI, what
Stan was suggesting below:

| Note, this is technically breaking existing UAPI where we used to
| return 1 and now will do -ENOBUFS. The alternative is to
| document that bpf_clone_redirect can return 1 for DROP and 2 for CN.

And then only adjusting the test case.

Programs checking for ret < 0 will continue to behave as before. Technically
the bpf_clone_redirect() did its job just that on the veth side things were
dropped. Other drivers such as tun, vrf, ipvlan, bond could already have
returned NET_XMIT_DROP, so technically it's not a new situation where it is
possible. And having a ret > 0 could then also be clearly used to differentiate
that something came from driver side rather than helper side.

>> For the selftest, may be another option is to use a 28 bytes data_in for the
>> lwt program redirecting to veth? 14 bytes used by bpf_prog_test_run_skb and
>> leave 14 bytes for veth_xmit. It seems the original intention of the "veth
>> ETH_HLEN+1 packet ingress" test is expecting it to succeed also.
> 
> IIUC, you're suggesting to pass full ipv4 or ipv6 packet for veth tests
> to make them actually succeed with the forwarding, right?
> 
> Sure, I can do that. But let's keep this entry with the -NOBUFS as well?
> Just for the sake of ensuring that we don't export NET_XMIT_xxx from
> uapi.
> 

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Stanislav Fomichev]

On 09/11, Daniel Borkmann wrote:
> On 9/11/23 7:11 PM, Stanislav Fomichev wrote:
> > On 09/09, Martin KaFai Lau wrote:
> > > On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
> > > > Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
> > > > packets") exposed the fact that bpf_clone_redirect is capable of
> > > > returning raw NET_XMIT_XXX return codes.
> > > > 
> > > > This is in the conflict with its UAPI doc which says the following:
> > > > "0 on success, or a negative error in case of failure."
> > > > 
> > > > Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
> > > > net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
> > > > as -ENOBUFS instead of 1.
> > > > 
> > > > Note, this is technically breaking existing UAPI where we used to
> > > > return 1 and now will do -ENOBUFS. The alternative is to
> > > > document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> > > > 
> > > > Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > ---
> > > >    net/core/filter.c | 3 +++
> > > >    1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > index a094694899c9..9e297931b02f 100644
> > > > --- a/net/core/filter.c
> > > > +++ b/net/core/filter.c
> > > > @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
> > > >    	ret = dev_queue_xmit(skb);
> > > >    	dev_xmit_recursion_dec();
> > > > +	if (ret > 0)
> > > > +		ret = net_xmit_errno(ret);
> > > 
> > > I think it is better to have bpf_clone_redirect returning -ENOBUFS instead
> > > of leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the
> > > uapi/bpf.h also mentions
> > > 
> > >   *      Return
> > >   *              0 on success, or a negative error in case of failure.
> > > 
> > > If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for
> > > __bpf_rx_skb? and should net_xmit_errno() only be done for
> > > bpf_clone_redirect()?  __bpf_{tx,rx}_skb is also used by skb_do_redirect()
> > > which also calls __bpf_redirect_neigh() that returns NET_XMIT_xxx but no
> > > caller seems to care the NET_XMIT_xxx value now.
> > 
> > __bpf_rx_skb seems to only add to backlog and doesn't seem to return any
> > of the NET_XMIT_xxx. But I might be wrong and haven't looked too deep
> > into that.
> > 
> > > Daniel should know more here. I would wait for Daniel to comment.
> > 
> > Ack, sure!
> 
> I think my preference would be to just document it in the helper UAPI, what
> Stan was suggesting below:
> 
> | Note, this is technically breaking existing UAPI where we used to
> | return 1 and now will do -ENOBUFS. The alternative is to
> | document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> 
> And then only adjusting the test case.

In this case, would we also need something similar to our
TCP_BPF_<state> changes? Like BUILD_BUG_ON(BPF_NET_XMIT_XXX !=
NET_XMIT_XXX)? Otherwise, we risk more leakage into the UAPI.
Merely documenting doesn't seem enough?

> Programs checking for ret < 0 will continue to behave as before. Technically
> the bpf_clone_redirect() did its job just that on the veth side things were
> dropped. Other drivers such as tun, vrf, ipvlan, bond could already have
> returned NET_XMIT_DROP, so technically it's not a new situation where it is
> possible. And having a ret > 0 could then also be clearly used to differentiate
> that something came from driver side rather than helper side.
> 
> > > For the selftest, may be another option is to use a 28 bytes data_in for the
> > > lwt program redirecting to veth? 14 bytes used by bpf_prog_test_run_skb and
> > > leave 14 bytes for veth_xmit. It seems the original intention of the "veth
> > > ETH_HLEN+1 packet ingress" test is expecting it to succeed also.
> > 
> > IIUC, you're suggesting to pass full ipv4 or ipv6 packet for veth tests
> > to make them actually succeed with the forwarding, right?
> > 
> > Sure, I can do that. But let's keep this entry with the -NOBUFS as well?
> > Just for the sake of ensuring that we don't export NET_XMIT_xxx from
> > uapi.
> > 
> 

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Martin KaFai Lau]

On 9/11/23 10:23 AM, Daniel Borkmann wrote:
> On 9/11/23 7:11 PM, Stanislav Fomichev wrote:
>> On 09/09, Martin KaFai Lau wrote:
>>> On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
>>>> Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
>>>> packets") exposed the fact that bpf_clone_redirect is capable of
>>>> returning raw NET_XMIT_XXX return codes.
>>>>
>>>> This is in the conflict with its UAPI doc which says the following:
>>>> "0 on success, or a negative error in case of failure."
>>>>
>>>> Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
>>>> net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
>>>> as -ENOBUFS instead of 1.
>>>>
>>>> Note, this is technically breaking existing UAPI where we used to
>>>> return 1 and now will do -ENOBUFS. The alternative is to
>>>> document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
>>>>
>>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>>> ---
>>>>    net/core/filter.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index a094694899c9..9e297931b02f 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, 
>>>> struct sk_buff *skb)
>>>>        ret = dev_queue_xmit(skb);
>>>>        dev_xmit_recursion_dec();
>>>> +    if (ret > 0)
>>>> +        ret = net_xmit_errno(ret);
>>>
>>> I think it is better to have bpf_clone_redirect returning -ENOBUFS instead
>>> of leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the
>>> uapi/bpf.h also mentions
>>>
>>>   *      Return
>>>   *              0 on success, or a negative error in case of failure.
>>>
>>> If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for
>>> __bpf_rx_skb? and should net_xmit_errno() only be done for
>>> bpf_clone_redirect()?  __bpf_{tx,rx}_skb is also used by skb_do_redirect()
>>> which also calls __bpf_redirect_neigh() that returns NET_XMIT_xxx but no
>>> caller seems to care the NET_XMIT_xxx value now.
>>
>> __bpf_rx_skb seems to only add to backlog and doesn't seem to return any
>> of the NET_XMIT_xxx. But I might be wrong and haven't looked too deep
>> into that.

enqueue_to_backlog could return NET_RX_DROP which happens to have the same value 
as NET_XMIT_DROP. I think this will get propagated back to __bpf_rx_skb().

>>
>>> Daniel should know more here. I would wait for Daniel to comment.
>>
>> Ack, sure!
> 
> I think my preference would be to just document it in the helper UAPI, what
> Stan was suggesting below:
> 
> | Note, this is technically breaking existing UAPI where we used to
> | return 1 and now will do -ENOBUFS. The alternative is to
> | document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> 
> And then only adjusting the test case.
> 
> Programs checking for ret < 0 will continue to behave as before. Technically
> the bpf_clone_redirect() did its job just that on the veth side things were
> dropped. Other drivers such as tun, vrf, ipvlan, bond could already have
> returned NET_XMIT_DROP, so technically it's not a new situation where it is
> possible. And having a ret > 0 could then also be clearly used to differentiate
> that something came from driver side rather than helper side.

sure. sgtm. Not sure if it will be useful to spell out the >0 meaning in uapi/bpf.h.

> 
>>> For the selftest, may be another option is to use a 28 bytes data_in for the
>>> lwt program redirecting to veth? 14 bytes used by bpf_prog_test_run_skb and
>>> leave 14 bytes for veth_xmit. It seems the original intention of the "veth
>>> ETH_HLEN+1 packet ingress" test is expecting it to succeed also.
>>
>> IIUC, you're suggesting to pass full ipv4 or ipv6 packet for veth tests
>> to make them actually succeed with the forwarding, right?
>>
>> Sure, I can do that. But let's keep this entry with the -NOBUFS as well?
>> Just for the sake of ensuring that we don't export NET_XMIT_xxx from
>> uapi.

In that case it makes sense to only change the eth+1byte test case to expect >0 
ret (or -ENOBUF as in patch 2, depending on the above discussion). No need to 
add an extra test.

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Daniel Borkmann]

On 9/11/23 7:41 PM, Stanislav Fomichev wrote:
> On 09/11, Daniel Borkmann wrote:
>> On 9/11/23 7:11 PM, Stanislav Fomichev wrote:
>>> On 09/09, Martin KaFai Lau wrote:
>>>> On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
>>>>> Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
>>>>> packets") exposed the fact that bpf_clone_redirect is capable of
>>>>> returning raw NET_XMIT_XXX return codes.
>>>>>
>>>>> This is in the conflict with its UAPI doc which says the following:
>>>>> "0 on success, or a negative error in case of failure."
>>>>>
>>>>> Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
>>>>> net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
>>>>> as -ENOBUFS instead of 1.
>>>>>
>>>>> Note, this is technically breaking existing UAPI where we used to
>>>>> return 1 and now will do -ENOBUFS. The alternative is to
>>>>> document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
>>>>>
>>>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>>>> ---
>>>>>     net/core/filter.c | 3 +++
>>>>>     1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>> index a094694899c9..9e297931b02f 100644
>>>>> --- a/net/core/filter.c
>>>>> +++ b/net/core/filter.c
>>>>> @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
>>>>>     	ret = dev_queue_xmit(skb);
>>>>>     	dev_xmit_recursion_dec();
>>>>> +	if (ret > 0)
>>>>> +		ret = net_xmit_errno(ret);
>>>>
>>>> I think it is better to have bpf_clone_redirect returning -ENOBUFS instead
>>>> of leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the
>>>> uapi/bpf.h also mentions
>>>>
>>>>    *      Return
>>>>    *              0 on success, or a negative error in case of failure.
>>>>
>>>> If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for
>>>> __bpf_rx_skb? and should net_xmit_errno() only be done for
>>>> bpf_clone_redirect()?  __bpf_{tx,rx}_skb is also used by skb_do_redirect()
>>>> which also calls __bpf_redirect_neigh() that returns NET_XMIT_xxx but no
>>>> caller seems to care the NET_XMIT_xxx value now.
>>>
>>> __bpf_rx_skb seems to only add to backlog and doesn't seem to return any
>>> of the NET_XMIT_xxx. But I might be wrong and haven't looked too deep
>>> into that.
>>>
>>>> Daniel should know more here. I would wait for Daniel to comment.
>>>
>>> Ack, sure!
>>
>> I think my preference would be to just document it in the helper UAPI, what
>> Stan was suggesting below:
>>
>> | Note, this is technically breaking existing UAPI where we used to
>> | return 1 and now will do -ENOBUFS. The alternative is to
>> | document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
>>
>> And then only adjusting the test case.
> 
> In this case, would we also need something similar to our
> TCP_BPF_<state> changes? Like BUILD_BUG_ON(BPF_NET_XMIT_XXX !=
> NET_XMIT_XXX)? Otherwise, we risk more leakage into the UAPI.
> Merely documenting doesn't seem enough?

We could probably just mention that a positive, non-zero code indicates
that the skb clone got forwarded to the target netdevice but got dropped
from driver side. This is somewhat also driver dependent e.g. if you look
at dummy which does drop-all, it returns NETDEV_TX_OK. Anything more
specific in the helper doc such as defining BPF_NET_XMIT_* would be more
confusing.

>> Programs checking for ret < 0 will continue to behave as before. Technically
>> the bpf_clone_redirect() did its job just that on the veth side things were
>> dropped. Other drivers such as tun, vrf, ipvlan, bond could already have
>> returned NET_XMIT_DROP, so technically it's not a new situation where it is
>> possible. And having a ret > 0 could then also be clearly used to differentiate
>> that something came from driver side rather than helper side.
>>
>>>> For the selftest, may be another option is to use a 28 bytes data_in for the
>>>> lwt program redirecting to veth? 14 bytes used by bpf_prog_test_run_skb and
>>>> leave 14 bytes for veth_xmit. It seems the original intention of the "veth
>>>> ETH_HLEN+1 packet ingress" test is expecting it to succeed also.
>>>
>>> IIUC, you're suggesting to pass full ipv4 or ipv6 packet for veth tests
>>> to make them actually succeed with the forwarding, right?
>>>
>>> Sure, I can do that. But let's keep this entry with the -NOBUFS as well?
>>> Just for the sake of ensuring that we don't export NET_XMIT_xxx from
>>> uapi.

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Stanislav Fomichev]

On 09/11, Daniel Borkmann wrote:
> On 9/11/23 7:41 PM, Stanislav Fomichev wrote:
> > On 09/11, Daniel Borkmann wrote:
> > > On 9/11/23 7:11 PM, Stanislav Fomichev wrote:
> > > > On 09/09, Martin KaFai Lau wrote:
> > > > > On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
> > > > > > Commit 151e887d8ff9 ("veth: Fixing transmit return status for dropped
> > > > > > packets") exposed the fact that bpf_clone_redirect is capable of
> > > > > > returning raw NET_XMIT_XXX return codes.
> > > > > > 
> > > > > > This is in the conflict with its UAPI doc which says the following:
> > > > > > "0 on success, or a negative error in case of failure."
> > > > > > 
> > > > > > Let's wrap dev_queue_xmit's return value (in __bpf_tx_skb) into
> > > > > > net_xmit_errno to make sure we correctly propagate NET_XMIT_DROP
> > > > > > as -ENOBUFS instead of 1.
> > > > > > 
> > > > > > Note, this is technically breaking existing UAPI where we used to
> > > > > > return 1 and now will do -ENOBUFS. The alternative is to
> > > > > > document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> > > > > > 
> > > > > > Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > > > ---
> > > > > >     net/core/filter.c | 3 +++
> > > > > >     1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > > index a094694899c9..9e297931b02f 100644
> > > > > > --- a/net/core/filter.c
> > > > > > +++ b/net/core/filter.c
> > > > > > @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
> > > > > >     	ret = dev_queue_xmit(skb);
> > > > > >     	dev_xmit_recursion_dec();
> > > > > > +	if (ret > 0)
> > > > > > +		ret = net_xmit_errno(ret);
> > > > > 
> > > > > I think it is better to have bpf_clone_redirect returning -ENOBUFS instead
> > > > > of leaking NET_XMIT_XXX to the uapi. The bpf_clone_redirect in the
> > > > > uapi/bpf.h also mentions
> > > > > 
> > > > >    *      Return
> > > > >    *              0 on success, or a negative error in case of failure.
> > > > > 
> > > > > If -ENOBUFS is returned in __bpf_tx_skb, should the same be done for
> > > > > __bpf_rx_skb? and should net_xmit_errno() only be done for
> > > > > bpf_clone_redirect()?  __bpf_{tx,rx}_skb is also used by skb_do_redirect()
> > > > > which also calls __bpf_redirect_neigh() that returns NET_XMIT_xxx but no
> > > > > caller seems to care the NET_XMIT_xxx value now.
> > > > 
> > > > __bpf_rx_skb seems to only add to backlog and doesn't seem to return any
> > > > of the NET_XMIT_xxx. But I might be wrong and haven't looked too deep
> > > > into that.
> > > > 
> > > > > Daniel should know more here. I would wait for Daniel to comment.
> > > > 
> > > > Ack, sure!
> > > 
> > > I think my preference would be to just document it in the helper UAPI, what
> > > Stan was suggesting below:
> > > 
> > > | Note, this is technically breaking existing UAPI where we used to
> > > | return 1 and now will do -ENOBUFS. The alternative is to
> > > | document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
> > > 
> > > And then only adjusting the test case.
> > 
> > In this case, would we also need something similar to our
> > TCP_BPF_<state> changes? Like BUILD_BUG_ON(BPF_NET_XMIT_XXX !=
> > NET_XMIT_XXX)? Otherwise, we risk more leakage into the UAPI.
> > Merely documenting doesn't seem enough?
> 
> We could probably just mention that a positive, non-zero code indicates
> that the skb clone got forwarded to the target netdevice but got dropped
> from driver side. This is somewhat also driver dependent e.g. if you look
> at dummy which does drop-all, it returns NETDEV_TX_OK. Anything more
> specific in the helper doc such as defining BPF_NET_XMIT_* would be more
> confusing.

Something like the following?

Return
	0 on success, or a negative error in case of failure. Positive
	error indicates a potential drop or congestion in the target
	device. The particular positive error codes are not defined.

Re: [PATCH bpf-next 1/2] bpf: return correct -ENOBUFS from bpf_clone_redirect

[Daniel Borkmann]

On 9/11/23 8:52 PM, Stanislav Fomichev wrote:
> On 09/11, Daniel Borkmann wrote:
>> On 9/11/23 7:41 PM, Stanislav Fomichev wrote:
>>> On 09/11, Daniel Borkmann wrote:
>>>> On 9/11/23 7:11 PM, Stanislav Fomichev wrote:
>>>>> On 09/09, Martin KaFai Lau wrote:
>>>>>> On 9/8/23 2:00 PM, Stanislav Fomichev wrote:
[...]
>>>> I think my preference would be to just document it in the helper UAPI, what
>>>> Stan was suggesting below:
>>>>
>>>> | Note, this is technically breaking existing UAPI where we used to
>>>> | return 1 and now will do -ENOBUFS. The alternative is to
>>>> | document that bpf_clone_redirect can return 1 for DROP and 2 for CN.
>>>>
>>>> And then only adjusting the test case.
>>>
>>> In this case, would we also need something similar to our
>>> TCP_BPF_<state> changes? Like BUILD_BUG_ON(BPF_NET_XMIT_XXX !=
>>> NET_XMIT_XXX)? Otherwise, we risk more leakage into the UAPI.
>>> Merely documenting doesn't seem enough?
>>
>> We could probably just mention that a positive, non-zero code indicates
>> that the skb clone got forwarded to the target netdevice but got dropped
>> from driver side. This is somewhat also driver dependent e.g. if you look
>> at dummy which does drop-all, it returns NETDEV_TX_OK. Anything more
>> specific in the helper doc such as defining BPF_NET_XMIT_* would be more
>> confusing.
> 
> Something like the following?
> 
> Return
> 	0 on success, or a negative error in case of failure. Positive
> 	error indicates a potential drop or congestion in the target
> 	device. The particular positive error codes are not defined.
> 

yeap, sgtm