| Message ID | 1335759088.20866.32.camel@pasglop |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Benjamin Herrenschmidt schrieb: > On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: > >>> Matt's having a look at powerpc >> >> Ok, he hasn't so I'll dig a bit. >> >> No obvious wrongness (but I'm not very familiar with bpf), though I do >> have a comment: sk_negative_common() and bpf_slow_path_common() should >> be made one and single macro which takes the fallback function as an >> argument. > > Ok, with the compile fix below it seems to work for me: > > (Feel free to fold that into the original patch) > Should i resend the complete patch with the compile fix? [snip] > > Cheers, > Ben. > Greetings Jan PS: I am sure i compile tested the orig. patch here, hmmm, must have lost that part when moving trees, #GitIsNotMyFriend
On Mon, 2012-04-30 at 06:27 +0200, Jan Seiffert wrote: > Benjamin Herrenschmidt schrieb: > > On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: > > > >>> Matt's having a look at powerpc > >> > >> Ok, he hasn't so I'll dig a bit. > >> > >> No obvious wrongness (but I'm not very familiar with bpf), though I do > >> have a comment: sk_negative_common() and bpf_slow_path_common() should > >> be made one and single macro which takes the fallback function as an > >> argument. > > > > Ok, with the compile fix below it seems to work for me: > > > > (Feel free to fold that into the original patch) > > > > Should i resend the complete patch with the compile fix? Won't hurt... BTW. Any idea about that bpf_program vs. sock_fprog issue I mentioned earlier ? Cheers, Ben. -- 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
Benjamin Herrenschmidt schrieb: > On Mon, 2012-04-30 at 06:27 +0200, Jan Seiffert wrote: >> Benjamin Herrenschmidt schrieb: >>> On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: >>> >>>>> Matt's having a look at powerpc >>>> >>>> Ok, he hasn't so I'll dig a bit. >>>> >>>> No obvious wrongness (but I'm not very familiar with bpf), though I do >>>> have a comment: sk_negative_common() and bpf_slow_path_common() should >>>> be made one and single macro which takes the fallback function as an >>>> argument. >>> >>> Ok, with the compile fix below it seems to work for me: >>> >>> (Feel free to fold that into the original patch) >>> >> >> Should i resend the complete patch with the compile fix? > > Won't hurt... > Ok > BTW. Any idea about that bpf_program vs. sock_fprog issue I mentioned > earlier ? > No idea, i was going by the old saying: "Thou shall not include kernel header, or you will feel the wrath of angry kernel gurus." > Cheers, > Ben. > Greetings Jan
> No idea, i was going by the old saying: > "Thou shall not include kernel header, or you will feel the wrath of angry > kernel gurus." Heh :-) Well, googling around, it looks like there's a mix of both type of programs out there. Those using a struct bpf_program and those using a struct soft_fprog. Only the latter will work on BE machines as far as I can tell. David, what's the right way to fix that ? Cheers, Ben. -- 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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Mon, 30 Apr 2012 15:26:08 +1000 > David, what's the right way to fix that ? There is no doubt that sock_fprog is the correct datastructure to use. -- 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
On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Date: Mon, 30 Apr 2012 15:26:08 +1000 > > > David, what's the right way to fix that ? > > There is no doubt that sock_fprog is the correct datastructure to use. Ok, so the right fix is to email anybody who posted code using struct bpf_program to fix their code ? :-) My question was more along the lines of should we attempt changing one of the two variants to make them match on BE (since they are in effect compatible on LE), tho of course this could have the usual annoying consequence of breaking the mangled c++ name of the symbol). From your reply I assume the answer is no... so that leaves us to chase up users and fix them. Great.... Cheers, Ben. -- 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
On Tue, 2012-05-01 at 07:55 +1000, Benjamin Herrenschmidt wrote: > On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote: > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Date: Mon, 30 Apr 2012 15:26:08 +1000 > > > > > David, what's the right way to fix that ? > > > > There is no doubt that sock_fprog is the correct datastructure to use. > > Ok, so the right fix is to email anybody who posted code using struct > bpf_program to fix their code ? :-) Actually, the right fix is for anybody using pcap-bpf.h to not use SO_ATTACH_FILTER directly but to use pcap_setfilter() which handles the compatibility. I'll start spamming web sites who tell people to do the wrong thing. Cheers, Ben. -- 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
Benjamin Herrenschmidt schrieb: > On Tue, 2012-05-01 at 07:55 +1000, Benjamin Herrenschmidt wrote: >> On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote: >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> Date: Mon, 30 Apr 2012 15:26:08 +1000 >>> >>>> David, what's the right way to fix that ? >>> >>> There is no doubt that sock_fprog is the correct datastructure to use. >> >> Ok, so the right fix is to email anybody who posted code using struct >> bpf_program to fix their code ? :-) > > Actually, the right fix is for anybody using pcap-bpf.h to not > use SO_ATTACH_FILTER directly but to use pcap_setfilter() which > handles the compatibility. > *shudder* Link to another lib for only one function because.... http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/net/bpf.h?rev=1.59&content-type=text/x-cvsweb-markup&only_with_tag=MAIN The "Original" says it's an u_int. But i guess it is unfixable without breaking something, except with ugly code. Should the padding at least be made explicit in the in-kernel struct? Did anyone ever tested the 32bit on 64bit compat code (different padding)? > I'll start spamming web sites who tell people to do the wrong thing. > > Cheers, > Ben. > Greetings Jan
On Tue, 2012-05-01 at 00:32 +0200, Jan Seiffert wrote: > *shudder* > Link to another lib for only one function because.... > > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/net/bpf.h?rev=1.59&content-type=text/x-cvsweb-markup&only_with_tag=MAIN > The "Original" says it's an u_int. > > But i guess it is unfixable without breaking something, except with ugly code. > Should the padding at least be made explicit in the in-kernel struct? > Did anyone ever tested the 32bit on 64bit compat code (different padding)? Haven't tested no. A quick google pointed to 2 web pages telling people to do the wrong thing and 4 broken programs out there, interestingly in different ways :-) (somebody's doing a reinterpret_cast of one struct into another, somebody does his/her own local redefinition of the struct ... using a ulong !, etc....) I don't see a good way to sort that out other than introducing a new kernel-side structure, change the sockopt number, and support the old one as backward binary compat, but that's gross. Cheers, Ben. -- 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
Benjamin Herrenschmidt schrieb: > On Tue, 2012-05-01 at 00:32 +0200, Jan Seiffert wrote: > >> *shudder* >> Link to another lib for only one function because.... >> >> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/net/bpf.h?rev=1.59&content-type=text/x-cvsweb-markup&only_with_tag=MAIN >> The "Original" says it's an u_int. >> >> But i guess it is unfixable without breaking something, except with ugly code. >> Should the padding at least be made explicit in the in-kernel struct? >> Did anyone ever tested the 32bit on 64bit compat code (different padding)? > > Haven't tested no. A quick google pointed to 2 web pages telling people > to do the wrong thing and 4 broken programs out there, interestingly in > different ways :-) (somebody's doing a reinterpret_cast of one struct > into another, somebody does his/her own local redefinition of the > struct ... using a ulong !, etc....) > Ouch! > I don't see a good way to sort that out other than introducing a new > kernel-side structure, change the sockopt number, and support the old > one as backward binary compat, but that's gross. > But a sane way forward? BTW, has anyone checked the new syscall filter code for these ... misfortunes? I mean before the ABI is set in stone... > Cheers, > Ben. > > > Greetings Jan
On Tue, 2012-05-01 at 02:44 +0200, Jan Seiffert wrote: > But a sane way forward? > > BTW, has anyone checked the new syscall filter code for these ... > misfortunes? > I mean before the ABI is set in stone... > Nope, not yet, looks like something I should do :-) Cheers, Ben. -- 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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Tue, 01 May 2012 10:26:27 +1000 > I don't see a good way to sort that out other than introducing a new > kernel-side structure, change the sockopt number, and support the old > one as backward binary compat, but that's gross. First there are no padding issues, compat or otherwise. Second, there is no reason to add a new data-structure. Just make them use the right one, sock_fprog, if they are making the socket option call themselves. I can't believe this discussion is still going on, there is nothing to talk about. -- 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
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h index af1ab5e..5c3cf2d 100644 --- a/arch/powerpc/net/bpf_jit.h +++ b/arch/powerpc/net/bpf_jit.h @@ -48,7 +48,13 @@ /* * Assembly helpers from arch/powerpc/net/bpf_jit.S: */ -extern u8 sk_load_word[], sk_load_half[], sk_load_byte[], sk_load_byte_msh[]; +#define DECLARE_LOAD_FUNC(func) \ + extern u8 func[], func##_negative_offset[], func##_positive_offset[] + +DECLARE_LOAD_FUNC(sk_load_word); +DECLARE_LOAD_FUNC(sk_load_half); +DECLARE_LOAD_FUNC(sk_load_byte); +DECLARE_LOAD_FUNC(sk_load_byte_msh); #define FUNCTION_DESCR_SIZE 24