Re: [PATCH] net: introduce SO_BPF_EXTENSIONS

[Daniel Borkmann <dborkman@redhat.com>]

On 01/16/2014 07:34 PM, Daniel Borkmann wrote:
> On 01/16/2014 06:26 PM, Michal Sekletar wrote:
>> On Thu, Jan 16, 2014 at 05:41:46PM +0100, Daniel Borkmann wrote:
>>> On 01/16/2014 05:14 PM, Michal Sekletar wrote:
>>>> userspace packet capturing libraries (e.g. libpcap) don't have a way how to find
>>>> out which BPF extensions are supported by the kernel, except compiling a filter
>>>> which uses extensions and trying to apply filter to the socket. This is very
>>>> inconvenient.
>>>>
>>>> Therefore this commit introduces new option which can be used as an argument for
>>>> getsockopt() and allows one to obtain information about which BPF extensions are
>>>> supported by the kernel.
>>>>
>>>> On Tue, Dec 10, 2013 at 1:25 AM, David Miller <davem@davemloft.net> wrote:
>>>>
>>>>> And therefore, you do not need to define any actual bits yet. The
>>>>> socket option will for now just return "0", and that will mean that
>>>>> all the extensions Linux implements currently are presnet.
>>>>
>>>> Signed-off-by: Michal Sekletar <msekleta@redhat.com>
>>>> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>>>> Cc: Daniel Borkmann <dborkman@redhat.com>
>>>> Cc: David Miller <davem@davemloft.net>
>>>> ---
>>>>   arch/alpha/include/uapi/asm/socket.h   | 2 ++
>>>>   arch/avr32/include/uapi/asm/socket.h   | 2 ++
>>>>   arch/cris/include/uapi/asm/socket.h    | 2 ++
>>>>   arch/frv/include/uapi/asm/socket.h     | 2 ++
>>>>   arch/ia64/include/uapi/asm/socket.h    | 2 ++
>>>>   arch/m32r/include/uapi/asm/socket.h    | 2 ++
>>>>   arch/mips/include/uapi/asm/socket.h    | 2 ++
>>>>   arch/mn10300/include/uapi/asm/socket.h | 2 ++
>>>>   arch/parisc/include/uapi/asm/socket.h  | 2 ++
>>>>   arch/powerpc/include/uapi/asm/socket.h | 2 ++
>>>>   arch/s390/include/uapi/asm/socket.h    | 2 ++
>>>>   arch/sparc/include/uapi/asm/socket.h   | 2 ++
>>>>   arch/xtensa/include/uapi/asm/socket.h  | 2 ++
>>>>   include/net/sock.h                     | 5 +++++
>>>>   include/uapi/asm-generic/socket.h      | 2 ++
>>>>   net/core/sock.c                        | 4 ++++
>>>>   16 files changed, 37 insertions(+)
>>>>
>>> ...
>>>> --- a/include/net/sock.h
>>>> +++ b/include/net/sock.h
>>>> @@ -2292,4 +2292,9 @@ extern int sysctl_optmem_max;
>>>>   extern __u32 sysctl_wmem_default;
>>>>   extern __u32 sysctl_rmem_default;
>>>>
>>>> +static inline int bpf_get_extensions(void)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>
>>> This should rather be in: include/linux/filter.h
>>
>> Yes, having that function there makes more sense.
>>
>>>
>>> And then, maybe be renamed into something like bpf_tell_extensions()
>>> for example, but that's just nit.
>>
>> Renamed.
>>
>>>
>>> Also, please add a comment, saying if people add new extensions, they
>>> need to enumerate them within this function from now on so that user
>>> space who enquires for that can be made aware of.
>>
>> Let me know if even more explanatory comment is desired.
>
> I think that looks good.

Ok, then it seems you need to put your changes on top of [1]; and
0x4029 is fine then.

Sine you have to rebase anyway, and since I just noticed in your v2
patch [2] ... In function bpf_tell_extensions(), you need to convert
your whitespaces to tab, see Documentation/CodingStyle. While you're
at it, also try to line-break the comment at around 80 chars.

Otherwise your patch looks good, thanks Michal.

   [1] http://patchwork.ozlabs.org/patch/311829/
   [2] http://patchwork.ozlabs.org/patch/311790/

>>> Still, grepping through latest libpcap sources, tells me, so far over
>>> the years they have included SKF_AD_PROTOCOL and SKF_AD_PKTTYPE, wow!
>>>
>>> That's very disappointing, so I have high hopes with this getsockopt().
>>>
>>> ;)
>>>
>>> Thanks,
>>
>> Thank you for the review. Btw, what do you think about define for parisc? I am
>
> Yes, saw that in the patch and had similar thoughts.
>
>> not sure I grok 0x4048 for SO_MAX_PACING_RATE and hence not sure about 0x4029
>> for SO_BPF_EXTENSIONS.
>
> Hmm, maybe typo; SO_MAX_PACING_RATE should have been a 0x4028 ?
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html