| Message ID | CA+55aFz-AD8rWb66rjN-YhAtPXukMiGSFfW3nBMCp8k1RYUvOQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 21, 2018 at 2:47 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > And other attribute specifiers we encourage people to put in other > parts of the type, like __user etc, so they don't have that same > parsing issue. Looking at other attributes we use on structs, we may have similar risks for these: __packed ____cacheline_aligned ____cacheline_aligned_in_smp ____cacheline_internodealigned_in_smp But they just haven't been used in places that we could trip over it as badly, AFAICT. > I guess one _extreme_ fix for this would be to put > > extern struct nostruct __randomize_layout; > > in our include/linux/kconfig.h, which I think we end up always > including first thanks to having it on the command line. We could do that for all the above, but I wonder if the real problem is our convention of using "regular" names for these kinds of attributes instead of parameterized names. If we always used something like: #define __struct(x) __attribute__(x) We'd avoid it, but we'd uglify our struct attributes: struct thing { ... } __struct(randomize_layout); though trying this now creates other problems. Hmmm. (Regardless, let me send the nfs fix separately...) -Kees > > Because if you do that, you actually get an error: > > CC [M] fs/nfsd/nfs4xdr.o > In file included from ./include/linux/fs_struct.h:5:0, > from fs/nfsd/nfs4xdr.c:36: > ./include/linux/path.h:11:3: error: conflicting types for ‘__randomize_layout’ > } __randomize_layout; > ^~~~~~~~~~~~~~~~~~ > In file included from <command-line>:0:0: > ././include/linux/kconfig.h:8:28: note: previous declaration of > ‘__randomize_layout’ was here > extern struct nostruct __randomize_layout; > ^~~~~~~~~~~~~~~~~~ > make[1]: *** [scripts/Makefile.build:317: fs/nfsd/nfs4xdr.o] Error 1 > > and we would have figured this out immediately. > > Broken example patch appended, in case somebody wants to play with > something like this or comes up with a better model entirely.. > > Linus > > --- > > diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h > index fec5076eda91..537dacb83380 100644 > --- a/include/linux/kconfig.h > +++ b/include/linux/kconfig.h > @@ -4,6 +4,10 @@ > > #include <generated/autoconf.h> > > +#ifndef __ASSEMBLY__ > + extern struct nostruct __randomize_layout; > +#endif > + > #define __ARG_PLACEHOLDER_1 0, > #define __take_second_arg(__ignored, val, ...) val
Hi Linus, 2018-02-22 7:47 GMT+09:00 Linus Torvalds <torvalds@linux-foundation.org>: > On Wed, Feb 21, 2018 at 2:19 PM, Maciej S. Szmigiero > <mail@maciej.szmigiero.name> wrote: >> >> One can see that offsets used to access various members of struct path are >> different, and also that the original file from step 3 contains an object >> named "__randomize_layout". > > Whee. > > Thanks for root-causing this issue, and this syntax of ours is clearly > *much* too fragile. > > We actually have similar issues with some of our other attributes, > where out nice "helpful" attribute shorthand can end up being just > silently interpreted as a variable name if they aren't defined in > time. > > For most of our other attributes, it just doesn't matter all that much > if some user doesn't happen to see the attribute. For > __randomize_layout, it's obviously very fatal, and silently just > generates crazy code. > > I'm not entirely sure what the right solution is, because it's > obviously much too easy to miss some #include by mistake. It's easy to > say "you should always include the proper header", but if a failure to > do so doesn't end up with any warnings or errors, but just silent bad > code generation, it's much too fragile. > > I wonder if we could change the syntax of that "__randomize_layout" > thing. Some of our related helper macros (ie > randomized_struct_fields_start/end) don't have the same problem, > because if you don't have the define for them, the compiler will > complain about bad syntax. > > And other attribute specifiers we encourage people to put in other > parts of the type, like __user etc, so they don't have that same > parsing issue. > > I guess one _extreme_ fix for this would be to put > > extern struct nostruct __randomize_layout; > > in our include/linux/kconfig.h, which I think we end up always > including first thanks to having it on the command line. > > Because if you do that, you actually get an error: > > CC [M] fs/nfsd/nfs4xdr.o > In file included from ./include/linux/fs_struct.h:5:0, > from fs/nfsd/nfs4xdr.c:36: > ./include/linux/path.h:11:3: error: conflicting types for ‘__randomize_layout’ > } __randomize_layout; > ^~~~~~~~~~~~~~~~~~ > In file included from <command-line>:0:0: > ././include/linux/kconfig.h:8:28: note: previous declaration of > ‘__randomize_layout’ was here > extern struct nostruct __randomize_layout; > ^~~~~~~~~~~~~~~~~~ > make[1]: *** [scripts/Makefile.build:317: fs/nfsd/nfs4xdr.o] Error 1 > > and we would have figured this out immediately. > > Broken example patch appended, in case somebody wants to play with > something like this or comes up with a better model entirely.. > > Linus > Sorry for chiming in late. I noticed this thread today, honestly, the commit made me upset. Can I suggest another way to make it less fragile? __attribute((...)) can be placed after 'struct'. So, we can write: struct __randomize_layout path { struct vfsmount *mnt; struct dentry *dentry; }; instead of struct path { struct vfsmount *mnt; struct dentry *dentry; } __randomize_layout; If we force the former notation, the undefined __randomize_layout results in a build error instead of silent broken code generation. It is true somebody can still place __randomize_layout after the closing brace, but can we check this by coccicheck or checkpatch.pl? (we can describe it in coding style documentation, of course) IMHO, we should not (ab)use include/linux/kconfig.h to bring in misc things.
On Mon, Mar 5, 2018 at 1:27 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Sorry for chiming in late. > > I noticed this thread today, > honestly, the commit made me upset. > > > Can I suggest another way to make it less fragile? > __attribute((...)) can be placed after 'struct'. > > > So, we can write: > > > struct __randomize_layout path { > struct vfsmount *mnt; > struct dentry *dentry; > }; > > > instead of > > > struct path { > struct vfsmount *mnt; > struct dentry *dentry; > } __randomize_layout; Ugh. I had tried this after the struct _name_, not after "struct" itself. This does fix it, though it remains fragile, as you mention. > If we force the former notation, > the undefined __randomize_layout results in a build error > instead of silent broken code generation. > > > It is true somebody can still place > __randomize_layout after the closing brace, > but can we check this by coccicheck or checkpatch.pl? > (we can describe it in coding style documentation, of course) > > > IMHO, we should not (ab)use include/linux/kconfig.h > to bring in misc things. I'm happy to send a patch that reverts the other changes and relocates all the markings... Linus, how would you like this to go? -Kees
On Mon, Mar 5, 2018 at 1:27 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Can I suggest another way to make it less fragile? > __attribute((...)) can be placed after 'struct'. That avoids the actual bug, but it wouldn't have helped _find_ the problem in the first place. If somebody ever does the same thing, they'd hit the same issue. And it's not just __randomize_struct, it's any of our other type markers. We can say "don't do that", but if there is no automated checking, it's still ripe to cause problems just because somebody didn't notice. So I'd rather have something that causes a build failure when something goes wrong, rather than silently accepting syntax that wasn't intended. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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/include/linux/kconfig.h b/include/linux/kconfig.h index fec5076eda91..537dacb83380 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -4,6 +4,10 @@ #include <generated/autoconf.h> +#ifndef __ASSEMBLY__ + extern struct nostruct __randomize_layout; +#endif + #define __ARG_PLACEHOLDER_1 0, #define __take_second_arg(__ignored, val, ...) val -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in