Hi,
I think this patch is causing user-space breakage, see [0].
The gist is that we do BPF_PROG_RUN of a socket filter with 14 byte input to determine whether
BPF_PROG_RUN is available or not. I'll fix this in cilium/ebpf, but I think this patch
needs more work since users may be doing the same thing in their code.
Thanks,
Lorenz
0: https://github.com/cilium/ebpf/pull/788
On Wed, Sep 14, 2022 at 4:20 AM Lorenz Bauer <[email protected]> wrote:
>
> Hi,
>
> I think this patch is causing user-space breakage, see [0].
>
> The gist is that we do BPF_PROG_RUN of a socket filter with 14 byte input to determine whether
> BPF_PROG_RUN is available or not. I'll fix this in cilium/ebpf, but I think this patch
> needs more work since users may be doing the same thing in their code.
Ooops, sorry about that.
Instead of rejecting len=0 data, we might accept the packet but add
some safe header? I think that should be more backwards compatible?
Zhengchao, something you can look into?
> Thanks,
> Lorenz
>
> 0: https://github.com/cilium/ebpf/pull/788
On 2022/9/17 23:46, Stanislav Fomichev wrote:
> On Wed, Sep 14, 2022 at 4:20 AM Lorenz Bauer <[email protected]> wrote:
>>
>> Hi,
>>
>> I think this patch is causing user-space breakage, see [0].
>>
>> The gist is that we do BPF_PROG_RUN of a socket filter with 14 byte input to determine whether
>> BPF_PROG_RUN is available or not. I'll fix this in cilium/ebpf, but I think this patch
>> needs more work since users may be doing the same thing in their code.
>
> Ooops, sorry about that.
>
> Instead of rejecting len=0 data, we might accept the packet but add
> some safe header? I think that should be more backwards compatible?
> Zhengchao, something you can look into?
>
>
Sorry for the delay. I'm busy testing the TC module recently. I'm very
sorry for the user-space breakage.
The root cause of this problem is that eth_type_trans() is called when
the protocol type of the SKB is parsed. The len value of the SKB is
reduced to 0. If the user mode requires that the forwarding succeed, or
if the MAC header is added again after the MAC header is subtracted,
is this appropriate?
Zhengchao Shao
>> Thanks,
>> Lorenz
>>
>> 0: https://github.com/cilium/ebpf/pull/788
On Mon, 19 Sep 2022, at 11:55, shaozhengchao wrote:
> Sorry for the delay. I'm busy testing the TC module recently. I'm very
> sorry for the user-space breakage.
>
> The root cause of this problem is that eth_type_trans() is called when
> the protocol type of the SKB is parsed. The len value of the SKB is
> reduced to 0. If the user mode requires that the forwarding succeed, or
> if the MAC header is added again after the MAC header is subtracted,
> is this appropriate?
We don't require forwarding to succeed with a 14 byte input buffer. We also don't look at the MAC header.
I think refusing to forward 0 length packets would be OK. Not 100% certain I understood you correctly, let me know if this helps.
Best
Lorenz
On 2022/9/20 22:42, Lorenz Bauer wrote:
> On Mon, 19 Sep 2022, at 11:55, shaozhengchao wrote:
>> Sorry for the delay. I'm busy testing the TC module recently. I'm very
>> sorry for the user-space breakage.
>>
>> The root cause of this problem is that eth_type_trans() is called when
>> the protocol type of the SKB is parsed. The len value of the SKB is
>> reduced to 0. If the user mode requires that the forwarding succeed, or
>> if the MAC header is added again after the MAC header is subtracted,
>> is this appropriate?
>
> We don't require forwarding to succeed with a 14 byte input buffer. We also don't look at the MAC header.
>
> I think refusing to forward 0 length packets would be OK. Not 100% certain I understood you correctly, let me know if this helps.
>
> Best
> Lorenz
Hi Lorenz
Sorry. But how does the rejection of the 0 length affect the
test case? Is the return value abnormal, send packet failure or some
others?
Zhengchao Shao
On Wed, Sep 21, 2022 at 1:48 AM shaozhengchao <[email protected]> wrote:
>
>
>
> On 2022/9/20 22:42, Lorenz Bauer wrote:
> > On Mon, 19 Sep 2022, at 11:55, shaozhengchao wrote:
> >> Sorry for the delay. I'm busy testing the TC module recently. I'm very
> >> sorry for the user-space breakage.
> >>
> >> The root cause of this problem is that eth_type_trans() is called when
> >> the protocol type of the SKB is parsed. The len value of the SKB is
> >> reduced to 0. If the user mode requires that the forwarding succeed, or
> >> if the MAC header is added again after the MAC header is subtracted,
> >> is this appropriate?
> >
> > We don't require forwarding to succeed with a 14 byte input buffer. We also don't look at the MAC header.
> >
> > I think refusing to forward 0 length packets would be OK. Not 100% certain I understood you correctly, let me know if this helps.
> >
> > Best
> > Lorenz
> Hi Lorenz
> Sorry. But how does the rejection of the 0 length affect the
> test case? Is the return value abnormal, send packet failure or some
> others?
Sorry for the late reply, I'm still traveling.
What we want to avoid is creating invalid skbs via prog_test_run.
We can reject <14 byte packets with ENIVAL, but that might break some
of the existing apps (as Lorenz reported).
So it seems like a safer alternative would be to accept those packets,
but just append missing bytes in the kernel to create the valid
packets? Padding with zeros seems fine?
> Zhengchao Shao
On Wed, 21 Sep 2022, at 09:48, shaozhengchao wrote:
> Hi Lorenz
> Sorry. But how does the rejection of the 0 length affect the
> test case? Is the return value abnormal, send packet failure or some
> others?
Hi Zhengchao,
Did you get around to submitting another fix for this?
Best
Lorenz
On 2022/10/13 17:36, Lorenz Bauer wrote:
> Did you get around to submitting another fix for this?
Hi Bauer:
Sorry, I haven't fully understood your intentions yet.
Can you explain it more detail?
Zhengchao Shao
On Thu, 13 Oct 2022, at 11:44, shaozhengchao wrote:
> Sorry, I haven't fully understood your intentions yet.
> Can you explain it more detail?
I'll try! Roughly, we do the following:
1. Create a BPF_PROG_TYPE_SOCKET_FILTER program that just returns 0
2. Load the program into the kernel
3. Call BPF_PROG_RUN with data_size_in == 14
After your bugfix, it seems like step 3 is rejected due to data_size_in == 14. We had to increase data_size_in to 15 to
avoid this, see [0].
This breaks user space, so it would be great if you could fix this in a way that doesn't refuse BPF_PROG_RUN with
data_size_in == 14. Since I don't understand the original problem very well I can't tell you what the best fix is however.
0: https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0
Thanks
Lorenz
On 10/14, Lorenz Bauer wrote:
> On Thu, 13 Oct 2022, at 11:44, shaozhengchao wrote:
> > Sorry, I haven't fully understood your intentions yet.
> > Can you explain it more detail?
> I'll try! Roughly, we do the following:
> 1. Create a BPF_PROG_TYPE_SOCKET_FILTER program that just returns 0
> 2. Load the program into the kernel
> 3. Call BPF_PROG_RUN with data_size_in == 14
> After your bugfix, it seems like step 3 is rejected due to data_size_in
> == 14. We had to increase data_size_in to 15 to
> avoid this, see [0].
> This breaks user space, so it would be great if you could fix this in a
> way that doesn't refuse BPF_PROG_RUN with
[..]
> data_size_in == 14. Since I don't understand the original problem very
> well I can't tell you what the best fix is however.
The problem was that we were able to generate skb with len=0 via
BPF_PROG_RUN. Prohibiting those cases breaks backwards compatibility, so
we either have to:
a) (preferred?) accept inputs with <14, but maybe internally pad to 14
bytes to make the core stack happy
b) revert the patch and instead have length checks at runtime; doesn't seem
to
be worth the penalty in the forwarding path because of some corner cases
like these ?
> 0:
> https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0
> Thanks
> Lorenz
On 2022/10/15 0:55, [email protected] wrote:
> On 10/14, Lorenz Bauer wrote:
>> On Thu, 13 Oct 2022, at 11:44, shaozhengchao wrote:
>> > Sorry, I haven't fully understood your intentions yet.
>> > Can you explain it more detail?
>
>> I'll try! Roughly, we do the following:
>
>> 1. Create a BPF_PROG_TYPE_SOCKET_FILTER program that just returns 0
>> 2. Load the program into the kernel
>> 3. Call BPF_PROG_RUN with data_size_in == 14
>
>> After your bugfix, it seems like step 3 is rejected due to
>> data_size_in == 14. We had to increase data_size_in to 15 to
>> avoid this, see [0].
>
>> This breaks user space, so it would be great if you could fix this in
>> a way that doesn't refuse BPF_PROG_RUN with
>
> [..]
>
>> data_size_in == 14. Since I don't understand the original problem very
>> well I can't tell you what the best fix is however.
>
> The problem was that we were able to generate skb with len=0 via
> BPF_PROG_RUN. Prohibiting those cases breaks backwards compatibility, so
> we either have to:
>
> a) (preferred?) accept inputs with <14, but maybe internally pad to 14
> bytes to make the core stack happy
> b) revert the patch and instead have length checks at runtime; doesn't
> seem to
> be worth the penalty in the forwarding path because of some corner cases
> like these ?
>
Hi sdf:
a） looks better and I'll put up a patch as soon as possible to
fix it.
Zhengchao Shao
>
>> 0:
>> https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0
>
>> Thanks
>> Lorenz