Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Tue, Dec 20, 2016 at 11:07 AM, Dan Carpenterwrote: > On Tue, Dec 20, 2016 at 02:57:17PM +, Hammond, John wrote: >> "{ NULL }" is valid ISO C, but unfortunately "{}" is not. > > In the kernel we don't care. We use lots of GCC extensions. We depend on the compiler to do "incomplete zero-initialization" of structures that are not mentioned in an initializer. The reason { NULL } works is because the first field in the structure can take a NULL value, and then the rest are zero-initialized by the compiler. { } is the same thing, but doesn't use ordered initialization. If this style is truly unacceptable to you, then { .somefield = NULL } can work, or as you point out, if it's being initialized later, the static initializer can be dropped entirely. -Kees -- Kees Cook Nexus Security
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Tue, Dec 20, 2016 at 11:07 AM, Dan Carpenter wrote: > On Tue, Dec 20, 2016 at 02:57:17PM +, Hammond, John wrote: >> "{ NULL }" is valid ISO C, but unfortunately "{}" is not. > > In the kernel we don't care. We use lots of GCC extensions. We depend on the compiler to do "incomplete zero-initialization" of structures that are not mentioned in an initializer. The reason { NULL } works is because the first field in the structure can take a NULL value, and then the rest are zero-initialized by the compiler. { } is the same thing, but doesn't use ordered initialization. If this style is truly unacceptable to you, then { .somefield = NULL } can work, or as you point out, if it's being initialized later, the static initializer can be dropped entirely. -Kees -- Kees Cook Nexus Security
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Tue, Dec 20, 2016 at 02:57:17PM +, Hammond, John wrote: > "{ NULL }" is valid ISO C, but unfortunately "{}" is not. In the kernel we don't care. We use lots of GCC extensions. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Tue, Dec 20, 2016 at 02:57:17PM +, Hammond, John wrote: > "{ NULL }" is valid ISO C, but unfortunately "{}" is not. In the kernel we don't care. We use lots of GCC extensions. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Tue, Dec 20, 2016 at 08:47:51AM -0800, Bruce Korb wrote: > > > > "{ NULL }" is valid ISO C, but unfortunately "{}" is not. > > Just make the thing "static const" and don't use an initializer. That also works, of course. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Tue, Dec 20, 2016 at 08:47:51AM -0800, Bruce Korb wrote: > > > > "{ NULL }" is valid ISO C, but unfortunately "{}" is not. > > Just make the thing "static const" and don't use an initializer. That also works, of course. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
> > "{ NULL }" is valid ISO C, but unfortunately "{}" is not. Just make the thing "static const" and don't use an initializer.
Re: [PATCH] staging: lustre: ldlm: use designated initializers
> > "{ NULL }" is valid ISO C, but unfortunately "{}" is not. Just make the thing "static const" and don't use an initializer.
RE: [PATCH] staging: lustre: ldlm: use designated initializers
> On Mon, Dec 19, 2016 at 08:47:50AM -0800, Bruce Korb wrote: > > On Mon, Dec 19, 2016 at 8:22 AM, James Simmons > > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > *req, __u64 *flags, > > >> int added = (mode == LCK_NL); > > >> int overlaps = 0; > > >> int splitted = 0; > > >> - const struct ldlm_callback_suite null_cbs = { NULL }; > > >> + const struct ldlm_callback_suite null_cbs = { }; > > >> > > >> CDEBUG(D_DLMTRACE, > > >> "flags %#llx owner %llu pid %u mode %u start %llu end > > >> %llu\n", > > > > > > Nak. Filling null_cbs with random data is a bad idea. If you look at > > > ldlm_lock_create() where this is used you have > > > > > > if (cbs) { > > > lock->l_blocking_ast = cbs->lcs_blocking; > > > lock->l_completion_ast = cbs->lcs_completion; > > > lock->l_glimpse_ast = cbs->lcs_glimpse; } > > > > > > Having lock->l_* point to random addresses is a bad idea. > > > What really needs to be done is proper initialization of that > > > structure. A bunch of patches will be coming to address this. > > > > I'm not understanding the effect of the original difference. If you > > specify any initializer, then all fields not specified are filled with > > zero bits. Any pointers are, perforce, NULL. That should make both "{ > > NULL }" and "{}" equivalent. > > They are equivalent, yes, but people want to use a GCC plugin that randomizes > struct layouts for internal structures and the plugin doesn't work when you > use > struct ordering to initialize the struct. The plugin requires that you use > designated intializers. "{ NULL }" is valid ISO C, but unfortunately "{}" is not.
RE: [PATCH] staging: lustre: ldlm: use designated initializers
> On Mon, Dec 19, 2016 at 08:47:50AM -0800, Bruce Korb wrote: > > On Mon, Dec 19, 2016 at 8:22 AM, James Simmons > > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > *req, __u64 *flags, > > >> int added = (mode == LCK_NL); > > >> int overlaps = 0; > > >> int splitted = 0; > > >> - const struct ldlm_callback_suite null_cbs = { NULL }; > > >> + const struct ldlm_callback_suite null_cbs = { }; > > >> > > >> CDEBUG(D_DLMTRACE, > > >> "flags %#llx owner %llu pid %u mode %u start %llu end > > >> %llu\n", > > > > > > Nak. Filling null_cbs with random data is a bad idea. If you look at > > > ldlm_lock_create() where this is used you have > > > > > > if (cbs) { > > > lock->l_blocking_ast = cbs->lcs_blocking; > > > lock->l_completion_ast = cbs->lcs_completion; > > > lock->l_glimpse_ast = cbs->lcs_glimpse; } > > > > > > Having lock->l_* point to random addresses is a bad idea. > > > What really needs to be done is proper initialization of that > > > structure. A bunch of patches will be coming to address this. > > > > I'm not understanding the effect of the original difference. If you > > specify any initializer, then all fields not specified are filled with > > zero bits. Any pointers are, perforce, NULL. That should make both "{ > > NULL }" and "{}" equivalent. > > They are equivalent, yes, but people want to use a GCC plugin that randomizes > struct layouts for internal structures and the plugin doesn't work when you > use > struct ordering to initialize the struct. The plugin requires that you use > designated intializers. "{ NULL }" is valid ISO C, but unfortunately "{}" is not.
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Mon, Dec 19, 2016 at 04:22:58PM +, James Simmons wrote: > > > Prepare to mark sensitive kernel structures for randomization by making > > sure they're using designated initializers. These were identified during > > allyesconfig builds of x86, arm, and arm64, with most initializer fixes > > extracted from grsecurity. > > > > Signed-off-by: Kees Cook> > --- > > drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > index 722160784f83..f815827532dc 100644 > > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > > *req, __u64 *flags, > > int added = (mode == LCK_NL); > > int overlaps = 0; > > int splitted = 0; > > - const struct ldlm_callback_suite null_cbs = { NULL }; > > + const struct ldlm_callback_suite null_cbs = { }; > > > > CDEBUG(D_DLMTRACE, > >"flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > Nak. Filling null_cbs with random data is a bad idea. You've misunderstood. The plugin just changes how the struct is laid out, it doesn't put data into the struct. So this is fine. The places where it's not fine are when the layout is required because it's shared with userspace or set by the hardware. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Mon, Dec 19, 2016 at 04:22:58PM +, James Simmons wrote: > > > Prepare to mark sensitive kernel structures for randomization by making > > sure they're using designated initializers. These were identified during > > allyesconfig builds of x86, arm, and arm64, with most initializer fixes > > extracted from grsecurity. > > > > Signed-off-by: Kees Cook > > --- > > drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > index 722160784f83..f815827532dc 100644 > > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > > *req, __u64 *flags, > > int added = (mode == LCK_NL); > > int overlaps = 0; > > int splitted = 0; > > - const struct ldlm_callback_suite null_cbs = { NULL }; > > + const struct ldlm_callback_suite null_cbs = { }; > > > > CDEBUG(D_DLMTRACE, > >"flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > Nak. Filling null_cbs with random data is a bad idea. You've misunderstood. The plugin just changes how the struct is laid out, it doesn't put data into the struct. So this is fine. The places where it's not fine are when the layout is required because it's shared with userspace or set by the hardware. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Mon, Dec 19, 2016 at 08:47:50AM -0800, Bruce Korb wrote: > On Mon, Dec 19, 2016 at 8:22 AM, James Simmons > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > >> *req, __u64 *flags, > >> int added = (mode == LCK_NL); > >> int overlaps = 0; > >> int splitted = 0; > >> - const struct ldlm_callback_suite null_cbs = { NULL }; > >> + const struct ldlm_callback_suite null_cbs = { }; > >> > >> CDEBUG(D_DLMTRACE, > >> "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > > > Nak. Filling null_cbs with random data is a bad idea. If you look at > > ldlm_lock_create() where this is used you have > > > > if (cbs) { > > lock->l_blocking_ast = cbs->lcs_blocking; > > lock->l_completion_ast = cbs->lcs_completion; > > lock->l_glimpse_ast = cbs->lcs_glimpse; > > } > > > > Having lock->l_* point to random addresses is a bad idea. > > What really needs to be done is proper initialization of that > > structure. A bunch of patches will be coming to address this. > > I'm not understanding the effect of the original difference. If you > specify any initializer, then all fields not specified are filled with > zero bits. Any pointers are, perforce, NULL. That should make both "{ > NULL }" and "{}" equivalent. They are equivalent, yes, but people want to use a GCC plugin that randomizes struct layouts for internal structures and the plugin doesn't work when you use struct ordering to initialize the struct. The plugin requires that you use designated intializers. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Mon, Dec 19, 2016 at 08:47:50AM -0800, Bruce Korb wrote: > On Mon, Dec 19, 2016 at 8:22 AM, James Simmons > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > >> *req, __u64 *flags, > >> int added = (mode == LCK_NL); > >> int overlaps = 0; > >> int splitted = 0; > >> - const struct ldlm_callback_suite null_cbs = { NULL }; > >> + const struct ldlm_callback_suite null_cbs = { }; > >> > >> CDEBUG(D_DLMTRACE, > >> "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > > > Nak. Filling null_cbs with random data is a bad idea. If you look at > > ldlm_lock_create() where this is used you have > > > > if (cbs) { > > lock->l_blocking_ast = cbs->lcs_blocking; > > lock->l_completion_ast = cbs->lcs_completion; > > lock->l_glimpse_ast = cbs->lcs_glimpse; > > } > > > > Having lock->l_* point to random addresses is a bad idea. > > What really needs to be done is proper initialization of that > > structure. A bunch of patches will be coming to address this. > > I'm not understanding the effect of the original difference. If you > specify any initializer, then all fields not specified are filled with > zero bits. Any pointers are, perforce, NULL. That should make both "{ > NULL }" and "{}" equivalent. They are equivalent, yes, but people want to use a GCC plugin that randomizes struct layouts for internal structures and the plugin doesn't work when you use struct ordering to initialize the struct. The plugin requires that you use designated intializers. regards, dan carpenter
Re: [PATCH] staging: lustre: ldlm: use designated initializers
> On Mon, Dec 19, 2016 at 8:22 AM, James Simmons > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > >> *req, __u64 *flags, > >> int added = (mode == LCK_NL); > >> int overlaps = 0; > >> int splitted = 0; > >> - const struct ldlm_callback_suite null_cbs = { NULL }; > >> + const struct ldlm_callback_suite null_cbs = { }; > >> > >> CDEBUG(D_DLMTRACE, > >> "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > > > Nak. Filling null_cbs with random data is a bad idea. If you look at > > ldlm_lock_create() where this is used you have > > > > if (cbs) { > > lock->l_blocking_ast = cbs->lcs_blocking; > > lock->l_completion_ast = cbs->lcs_completion; > > lock->l_glimpse_ast = cbs->lcs_glimpse; > > } > > > > Having lock->l_* point to random addresses is a bad idea. > > What really needs to be done is proper initialization of that > > structure. A bunch of patches will be coming to address this. > > I'm not understanding the effect of the original difference. If you > specify any initializer, then all fields not specified are filled with > zero bits. Any pointers are, perforce, NULL. That should make both "{ > NULL }" and "{}" equivalent. Maybe a worthwhile change would be to: > > static const struct ldlm_callback_suite null_cbs; I perfer this as well. > then it is not even necessary to specify an initializer.
Re: [PATCH] staging: lustre: ldlm: use designated initializers
> On Mon, Dec 19, 2016 at 8:22 AM, James Simmons > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > >> *req, __u64 *flags, > >> int added = (mode == LCK_NL); > >> int overlaps = 0; > >> int splitted = 0; > >> - const struct ldlm_callback_suite null_cbs = { NULL }; > >> + const struct ldlm_callback_suite null_cbs = { }; > >> > >> CDEBUG(D_DLMTRACE, > >> "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > > > Nak. Filling null_cbs with random data is a bad idea. If you look at > > ldlm_lock_create() where this is used you have > > > > if (cbs) { > > lock->l_blocking_ast = cbs->lcs_blocking; > > lock->l_completion_ast = cbs->lcs_completion; > > lock->l_glimpse_ast = cbs->lcs_glimpse; > > } > > > > Having lock->l_* point to random addresses is a bad idea. > > What really needs to be done is proper initialization of that > > structure. A bunch of patches will be coming to address this. > > I'm not understanding the effect of the original difference. If you > specify any initializer, then all fields not specified are filled with > zero bits. Any pointers are, perforce, NULL. That should make both "{ > NULL }" and "{}" equivalent. Maybe a worthwhile change would be to: > > static const struct ldlm_callback_suite null_cbs; I perfer this as well. > then it is not even necessary to specify an initializer.
Re: [lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers
> James, > > > This should be a purely syntactical change, to help out tools - for GCC, I'm > pretty sure the meaning of { > } and { NULL } are the same. Also, I don't think struct randomization does > what you're thinking. Yep. I misread his commit message. That is why it didn't make sense to me. > Is there anything written up on kernel struct randomization? I was trying to > find a talk/post from you or > something from LWN, but I couldn't find something about this specifically. > (Probably because I can't find > it among the other stuff that's been written up) > > > - Patrick > > __ > From: lustre-devel <lustre-devel-boun...@lists.lustre.org> on behalf of James > Simmons > <jsimm...@infradead.org> > Sent: Monday, December 19, 2016 10:22:58 AM > To: Kees Cook > Cc: de...@driverdev.osuosl.org; Greg Kroah-Hartman; > linux-kernel@vger.kernel.org; Oleg Drokin; Vitaly > Fertman; Bruce Korb; Emoly Liu; lustre-de...@lists.lustre.org > Subject: Re: [lustre-devel] [PATCH] staging: lustre: ldlm: use designated > initializers > > > Prepare to mark sensitive kernel structures for randomization by making > > sure they're using designated initializers. These were identified during > > allyesconfig builds of x86, arm, and arm64, with most initializer fixes > > extracted from grsecurity. > > > > Signed-off-by: Kees Cook <keesc...@chromium.org> > > --- > > drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > index 722160784f83..f815827532dc 100644 > > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > > *req, __u64 *flags, > > int added = (mode == LCK_NL); > > int overlaps = 0; > > int splitted = 0; > > - const struct ldlm_callback_suite null_cbs = { NULL }; > > + const struct ldlm_callback_suite null_cbs = { }; > > > > CDEBUG(D_DLMTRACE, > > "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > Nak. Filling null_cbs with random data is a bad idea. If you look at > ldlm_lock_create() where this is used you have > > if (cbs) { > lock->l_blocking_ast = cbs->lcs_blocking; > lock->l_completion_ast = cbs->lcs_completion; > lock->l_glimpse_ast = cbs->lcs_glimpse; > } > > Having lock->l_* point to random addresses is a bad idea. > What really needs to be done is proper initialization of that > structure. A bunch of patches will be coming to address this. > ___ > lustre-devel mailing list > lustre-de...@lists.lustre.org > http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > >
Re: [lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers
> James, > > > This should be a purely syntactical change, to help out tools - for GCC, I'm > pretty sure the meaning of { > } and { NULL } are the same. Also, I don't think struct randomization does > what you're thinking. Yep. I misread his commit message. That is why it didn't make sense to me. > Is there anything written up on kernel struct randomization? I was trying to > find a talk/post from you or > something from LWN, but I couldn't find something about this specifically. > (Probably because I can't find > it among the other stuff that's been written up) > > > - Patrick > > __ > From: lustre-devel on behalf of James > Simmons > > Sent: Monday, December 19, 2016 10:22:58 AM > To: Kees Cook > Cc: de...@driverdev.osuosl.org; Greg Kroah-Hartman; > linux-kernel@vger.kernel.org; Oleg Drokin; Vitaly > Fertman; Bruce Korb; Emoly Liu; lustre-de...@lists.lustre.org > Subject: Re: [lustre-devel] [PATCH] staging: lustre: ldlm: use designated > initializers > > > Prepare to mark sensitive kernel structures for randomization by making > > sure they're using designated initializers. These were identified during > > allyesconfig builds of x86, arm, and arm64, with most initializer fixes > > extracted from grsecurity. > > > > Signed-off-by: Kees Cook > > --- > > drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > index 722160784f83..f815827532dc 100644 > > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock > > *req, __u64 *flags, > > int added = (mode == LCK_NL); > > int overlaps = 0; > > int splitted = 0; > > - const struct ldlm_callback_suite null_cbs = { NULL }; > > + const struct ldlm_callback_suite null_cbs = { }; > > > > CDEBUG(D_DLMTRACE, > > "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > Nak. Filling null_cbs with random data is a bad idea. If you look at > ldlm_lock_create() where this is used you have > > if (cbs) { > lock->l_blocking_ast = cbs->lcs_blocking; > lock->l_completion_ast = cbs->lcs_completion; > lock->l_glimpse_ast = cbs->lcs_glimpse; > } > > Having lock->l_* point to random addresses is a bad idea. > What really needs to be done is proper initialization of that > structure. A bunch of patches will be coming to address this. > ___ > lustre-devel mailing list > lustre-de...@lists.lustre.org > http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org > >
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Mon, Dec 19, 2016 at 8:22 AM, James Simmons >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock >> *req, __u64 *flags, >> int added = (mode == LCK_NL); >> int overlaps = 0; >> int splitted = 0; >> - const struct ldlm_callback_suite null_cbs = { NULL }; >> + const struct ldlm_callback_suite null_cbs = { }; >> >> CDEBUG(D_DLMTRACE, >> "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > Nak. Filling null_cbs with random data is a bad idea. If you look at > ldlm_lock_create() where this is used you have > > if (cbs) { > lock->l_blocking_ast = cbs->lcs_blocking; > lock->l_completion_ast = cbs->lcs_completion; > lock->l_glimpse_ast = cbs->lcs_glimpse; > } > > Having lock->l_* point to random addresses is a bad idea. > What really needs to be done is proper initialization of that > structure. A bunch of patches will be coming to address this. I'm not understanding the effect of the original difference. If you specify any initializer, then all fields not specified are filled with zero bits. Any pointers are, perforce, NULL. That should make both "{ NULL }" and "{}" equivalent. Maybe a worthwhile change would be to: static const struct ldlm_callback_suite null_cbs; then it is not even necessary to specify an initializer.
Re: [PATCH] staging: lustre: ldlm: use designated initializers
On Mon, Dec 19, 2016 at 8:22 AM, James Simmons >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock >> *req, __u64 *flags, >> int added = (mode == LCK_NL); >> int overlaps = 0; >> int splitted = 0; >> - const struct ldlm_callback_suite null_cbs = { NULL }; >> + const struct ldlm_callback_suite null_cbs = { }; >> >> CDEBUG(D_DLMTRACE, >> "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", > > Nak. Filling null_cbs with random data is a bad idea. If you look at > ldlm_lock_create() where this is used you have > > if (cbs) { > lock->l_blocking_ast = cbs->lcs_blocking; > lock->l_completion_ast = cbs->lcs_completion; > lock->l_glimpse_ast = cbs->lcs_glimpse; > } > > Having lock->l_* point to random addresses is a bad idea. > What really needs to be done is proper initialization of that > structure. A bunch of patches will be coming to address this. I'm not understanding the effect of the original difference. If you specify any initializer, then all fields not specified are filled with zero bits. Any pointers are, perforce, NULL. That should make both "{ NULL }" and "{}" equivalent. Maybe a worthwhile change would be to: static const struct ldlm_callback_suite null_cbs; then it is not even necessary to specify an initializer.
Re: [PATCH] staging: lustre: ldlm: use designated initializers
> Prepare to mark sensitive kernel structures for randomization by making > sure they're using designated initializers. These were identified during > allyesconfig builds of x86, arm, and arm64, with most initializer fixes > extracted from grsecurity. > > Signed-off-by: Kees Cook> --- > drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > index 722160784f83..f815827532dc 100644 > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, > __u64 *flags, > int added = (mode == LCK_NL); > int overlaps = 0; > int splitted = 0; > - const struct ldlm_callback_suite null_cbs = { NULL }; > + const struct ldlm_callback_suite null_cbs = { }; > > CDEBUG(D_DLMTRACE, > "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", Nak. Filling null_cbs with random data is a bad idea. If you look at ldlm_lock_create() where this is used you have if (cbs) { lock->l_blocking_ast = cbs->lcs_blocking; lock->l_completion_ast = cbs->lcs_completion; lock->l_glimpse_ast = cbs->lcs_glimpse; } Having lock->l_* point to random addresses is a bad idea. What really needs to be done is proper initialization of that structure. A bunch of patches will be coming to address this.
Re: [PATCH] staging: lustre: ldlm: use designated initializers
> Prepare to mark sensitive kernel structures for randomization by making > sure they're using designated initializers. These were identified during > allyesconfig builds of x86, arm, and arm64, with most initializer fixes > extracted from grsecurity. > > Signed-off-by: Kees Cook > --- > drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > index 722160784f83..f815827532dc 100644 > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, > __u64 *flags, > int added = (mode == LCK_NL); > int overlaps = 0; > int splitted = 0; > - const struct ldlm_callback_suite null_cbs = { NULL }; > + const struct ldlm_callback_suite null_cbs = { }; > > CDEBUG(D_DLMTRACE, > "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", Nak. Filling null_cbs with random data is a bad idea. If you look at ldlm_lock_create() where this is used you have if (cbs) { lock->l_blocking_ast = cbs->lcs_blocking; lock->l_completion_ast = cbs->lcs_completion; lock->l_glimpse_ast = cbs->lcs_glimpse; } Having lock->l_* point to random addresses is a bad idea. What really needs to be done is proper initialization of that structure. A bunch of patches will be coming to address this.
[PATCH] staging: lustre: ldlm: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making sure they're using designated initializers. These were identified during allyesconfig builds of x86, arm, and arm64, with most initializer fixes extracted from grsecurity. Signed-off-by: Kees Cook--- drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c index 722160784f83..f815827532dc 100644 --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags, int added = (mode == LCK_NL); int overlaps = 0; int splitted = 0; - const struct ldlm_callback_suite null_cbs = { NULL }; + const struct ldlm_callback_suite null_cbs = { }; CDEBUG(D_DLMTRACE, "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", -- 2.7.4 -- Kees Cook Nexus Security
[PATCH] staging: lustre: ldlm: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making sure they're using designated initializers. These were identified during allyesconfig builds of x86, arm, and arm64, with most initializer fixes extracted from grsecurity. Signed-off-by: Kees Cook --- drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c index 722160784f83..f815827532dc 100644 --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags, int added = (mode == LCK_NL); int overlaps = 0; int splitted = 0; - const struct ldlm_callback_suite null_cbs = { NULL }; + const struct ldlm_callback_suite null_cbs = { }; CDEBUG(D_DLMTRACE, "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n", -- 2.7.4 -- Kees Cook Nexus Security