On Tue, Jul 18, 2023 at 9:06 AM Jakub Kicinski kuba@kernel.org wrote:
On Tue, 18 Jul 2023 08:52:55 -0700 Alexei Starovoitov wrote:
On Tue, Jul 18, 2023 at 8:26 AM Jakub Kicinski kuba@kernel.org wrote:
On Mon, 1 May 2023 17:52:16 -0700 Daniel Rosenberg wrote:
--- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -4033,7 +4033,7 @@ __skb_header_pointer(const struct sk_buff *skb, int offset, int len, if (likely(hlen - offset >= len)) return (void *)data + offset;
if (!skb || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0))
if (!skb || !buffer || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0)) return NULL;First off - please make sure you CC netdev on changes to networking!
Please do not add stupid error checks to core code for BPF safety. Wrap the call if you can't guarantee that value is sane, this is a very bad precedent.
This is NOT for safety. You misread the code.
Doesn't matter, safety or optionality. skb_header_pointer() is used on the fast paths of the networking stack, adding heavy handed input validation to it is not okay. No sane code should be passing NULL buffer to skb_header_pointer(). Please move the NULL check to the BPF code so the rest of the networking stack does not have to pay the cost.
This should be common sense. If one caller is doing something.. "special" the extra code should live in the caller, not the callee. That's basic code hygiene.
you're still missing the point. Pls read the whole patch series. It is _not_ input validation. skb_copy_bits is a slow path. One extra check doesn't affect performance at all. So 'fast paths' isn't a valid argument here. The code is reusing if (likely(hlen - offset >= len)) return (void *)data + offset; which _is_ the fast path.
What you're requesting is to copy paste the whole __skb_header_pointer into __skb_header_pointer2. Makes no sense.