From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH] net: introduce SO_BPF_EXTENSIONS Date: Thu, 16 Jan 2014 20:24:34 +0100 Message-ID: <52D831F2.2030209@redhat.com> References: <1389888898-31744-1-git-send-email-msekleta@redhat.com> <52D80BCA.7030400@redhat.com> <20140116172631.GA5206@loki.brq.redhat.com> <52D82626.5060701@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Michael Kerrisk , David Miller , Eric Dumazet To: Michal Sekletar Return-path: Received: from mx1.redhat.com ([209.132.183.28]:23744 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750994AbaAPTYl (ORCPT ); Thu, 16 Jan 2014 14:24:41 -0500 In-Reply-To: <52D82626.5060701@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 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 >>>> Cc: Michael Kerrisk >>>> Cc: Daniel Borkmann >>>> Cc: David Miller >>>> --- >>>> 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