* [PATCH] staging: sm750fb: Make variable read only after initialization. @ 2016-10-17 2:32 Elise Lennion 2016-10-17 7:44 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Elise Lennion @ 2016-10-17 2:32 UTC (permalink / raw) To: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel Use __ro_after_init to protect variable, initialized in several steps, from overwrite during runtime. Signed-off-by: Elise Lennion <elise.lennion@gmail.com> --- drivers/staging/sm750fb/sm750.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c index 0663ec0..5c02ad8 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -719,7 +719,7 @@ static int sm750fb_set_drv(struct lynxfb_par *par) return ret; } -static struct fb_ops lynxfb_ops = { +static __ro_after_init struct fb_ops lynxfb_ops = { .owner = THIS_MODULE, .fb_check_var = lynxfb_ops_check_var, .fb_set_par = lynxfb_ops_set_par, -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: sm750fb: Make variable read only after initialization. 2016-10-17 2:32 [PATCH] staging: sm750fb: Make variable read only after initialization Elise Lennion @ 2016-10-17 7:44 ` Greg KH 2016-10-18 21:46 ` Elise Lennion 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2016-10-17 7:44 UTC (permalink / raw) To: Elise Lennion; +Cc: sudipm.mukherjee, teddy.wang, outreachy-kernel On Mon, Oct 17, 2016 at 12:32:16AM -0200, Elise Lennion wrote: > Use __ro_after_init to protect variable, initialized in several steps, > from overwrite during runtime. > > Signed-off-by: Elise Lennion <elise.lennion@gmail.com> > --- > drivers/staging/sm750fb/sm750.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > index 0663ec0..5c02ad8 100644 > --- a/drivers/staging/sm750fb/sm750.c > +++ b/drivers/staging/sm750fb/sm750.c > @@ -719,7 +719,7 @@ static int sm750fb_set_drv(struct lynxfb_par *par) > return ret; > } > > -static struct fb_ops lynxfb_ops = { > +static __ro_after_init struct fb_ops lynxfb_ops = { > .owner = THIS_MODULE, > .fb_check_var = lynxfb_ops_check_var, > .fb_set_par = lynxfb_ops_set_par, How did you "prove" or test this? I don't think it is correct to mark this code this way at all, but would be glad to reconsider the patch if you can prove me otherwise :) thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: sm750fb: Make variable read only after initialization. 2016-10-17 7:44 ` Greg KH @ 2016-10-18 21:46 ` Elise Lennion 2016-10-19 6:00 ` [Outreachy kernel] " Julia Lawall 0 siblings, 1 reply; 6+ messages in thread From: Elise Lennion @ 2016-10-18 21:46 UTC (permalink / raw) To: Greg KH; +Cc: sudipm.mukherjee, teddy.wang, outreachy-kernel On Mon, Oct 17, 2016 at 09:44:04AM +0200, Greg KH wrote: > On Mon, Oct 17, 2016 at 12:32:16AM -0200, Elise Lennion wrote: > > Use __ro_after_init to protect variable, initialized in several steps, > > from overwrite during runtime. > > > > Signed-off-by: Elise Lennion <elise.lennion@gmail.com> > > --- > > drivers/staging/sm750fb/sm750.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > > index 0663ec0..5c02ad8 100644 > > --- a/drivers/staging/sm750fb/sm750.c > > +++ b/drivers/staging/sm750fb/sm750.c > > @@ -719,7 +719,7 @@ static int sm750fb_set_drv(struct lynxfb_par *par) > > return ret; > > } > > > > -static struct fb_ops lynxfb_ops = { > > +static __ro_after_init struct fb_ops lynxfb_ops = { > > .owner = THIS_MODULE, > > .fb_check_var = lynxfb_ops_check_var, > > .fb_set_par = lynxfb_ops_set_par, > > How did you "prove" or test this? I don't think it is correct to mark > this code this way at all, but would be glad to reconsider the patch if > you can prove me otherwise :) > > thanks, > > greg k-h I compiled the file, used 'size' to verify that the data segment size was smaller, installed the module, removed and reloaded the module with modprobe and used lsmod to ensure the driver was loaded. Actually I just compiled the module and checked the data segment size before sending the patch, but now I did the whole procedure and everything seems right :) But I didn't find a good reference about how __ro_after_init works, only a few old patches that use it, so I don't really know if it can be loaded and still not have the desired effect, make lynxfb_ops read only after initialization. I put it in that position because that's where a const would be, but I searched more patches and most of them use __ro_after_init after the name of the variable. Is that what's strange in this patch? I'll make a v2 with __ro_after_init in the standard position in case that's the problem. thank you, elise ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] staging: sm750fb: Make variable read only after initialization. 2016-10-18 21:46 ` Elise Lennion @ 2016-10-19 6:00 ` Julia Lawall 2016-10-19 6:48 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2016-10-19 6:00 UTC (permalink / raw) To: Elise Lennion; +Cc: Greg KH, sudipm.mukherjee, teddy.wang, outreachy-kernel On Tue, 18 Oct 2016, Elise Lennion wrote: > On Mon, Oct 17, 2016 at 09:44:04AM +0200, Greg KH wrote: > > On Mon, Oct 17, 2016 at 12:32:16AM -0200, Elise Lennion wrote: > > > Use __ro_after_init to protect variable, initialized in several steps, > > > from overwrite during runtime. > > > > > > Signed-off-by: Elise Lennion <elise.lennion@gmail.com> > > > --- > > > drivers/staging/sm750fb/sm750.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > > > index 0663ec0..5c02ad8 100644 > > > --- a/drivers/staging/sm750fb/sm750.c > > > +++ b/drivers/staging/sm750fb/sm750.c > > > @@ -719,7 +719,7 @@ static int sm750fb_set_drv(struct lynxfb_par *par) > > > return ret; > > > } > > > > > > -static struct fb_ops lynxfb_ops = { > > > +static __ro_after_init struct fb_ops lynxfb_ops = { > > > .owner = THIS_MODULE, > > > .fb_check_var = lynxfb_ops_check_var, > > > .fb_set_par = lynxfb_ops_set_par, > > > > How did you "prove" or test this? I don't think it is correct to mark > > this code this way at all, but would be glad to reconsider the patch if > > you can prove me otherwise :) > > > > thanks, > > > > greg k-h > > I compiled the file, used 'size' to verify that the data segment size > was smaller, installed the module, removed and reloaded the module with > modprobe and used lsmod to ensure the driver was loaded. > > Actually I just compiled the module and checked the data segment size > before sending the patch, but now I did the whole procedure and > everything seems right :) > > But I didn't find a good reference about how __ro_after_init works, > only a few old patches that use it, so I don't really know if it can > be loaded and still not have the desired effect, make lynxfb_ops read > only after initialization. > > I put it in that position because that's where a const would be, but > I searched more patches and most of them use __ro_after_init after the > name of the variable. Is that what's strange in this patch? > > I'll make a v2 with __ro_after_init in the standard position in case > that's the problem. I think it would be helpful to give an overview of how the structure is actually used in practice. Eg, it is only modified in function XX which is declared as __init, and otherwise it is only passed to function YY where the corresponding parameter is declared as const. Checking that the declaration is really compiled is good, but it doesn't check that all the uses are compiled in your configuration, so it is helpful to check the problem from multiple perspectives. julia ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] staging: sm750fb: Make variable read only after initialization. 2016-10-19 6:00 ` [Outreachy kernel] " Julia Lawall @ 2016-10-19 6:48 ` Greg KH 2016-10-20 23:23 ` Elise Lennion 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2016-10-19 6:48 UTC (permalink / raw) To: Julia Lawall Cc: Elise Lennion, sudipm.mukherjee, teddy.wang, outreachy-kernel On Wed, Oct 19, 2016 at 08:00:15AM +0200, Julia Lawall wrote: > > > On Tue, 18 Oct 2016, Elise Lennion wrote: > > > On Mon, Oct 17, 2016 at 09:44:04AM +0200, Greg KH wrote: > > > On Mon, Oct 17, 2016 at 12:32:16AM -0200, Elise Lennion wrote: > > > > Use __ro_after_init to protect variable, initialized in several steps, > > > > from overwrite during runtime. > > > > > > > > Signed-off-by: Elise Lennion <elise.lennion@gmail.com> > > > > --- > > > > drivers/staging/sm750fb/sm750.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > > > > index 0663ec0..5c02ad8 100644 > > > > --- a/drivers/staging/sm750fb/sm750.c > > > > +++ b/drivers/staging/sm750fb/sm750.c > > > > @@ -719,7 +719,7 @@ static int sm750fb_set_drv(struct lynxfb_par *par) > > > > return ret; > > > > } > > > > > > > > -static struct fb_ops lynxfb_ops = { > > > > +static __ro_after_init struct fb_ops lynxfb_ops = { > > > > .owner = THIS_MODULE, > > > > .fb_check_var = lynxfb_ops_check_var, > > > > .fb_set_par = lynxfb_ops_set_par, > > > > > > How did you "prove" or test this? I don't think it is correct to mark > > > this code this way at all, but would be glad to reconsider the patch if > > > you can prove me otherwise :) > > > > > > thanks, > > > > > > greg k-h > > > > I compiled the file, used 'size' to verify that the data segment size > > was smaller, installed the module, removed and reloaded the module with > > modprobe and used lsmod to ensure the driver was loaded. > > > > Actually I just compiled the module and checked the data segment size > > before sending the patch, but now I did the whole procedure and > > everything seems right :) > > > > But I didn't find a good reference about how __ro_after_init works, > > only a few old patches that use it, so I don't really know if it can > > be loaded and still not have the desired effect, make lynxfb_ops read > > only after initialization. > > > > I put it in that position because that's where a const would be, but > > I searched more patches and most of them use __ro_after_init after the > > name of the variable. Is that what's strange in this patch? > > > > I'll make a v2 with __ro_after_init in the standard position in case > > that's the problem. > > I think it would be helpful to give an overview of how the structure is > actually used in practice. Eg, it is only modified in function XX which > is declared as __init, and otherwise it is only passed to function YY > where the corresponding parameter is declared as const. Checking that the > declaration is really compiled is good, but it doesn't check that all the > uses are compiled in your configuration, so it is helpful to check the > problem from multiple perspectives. Along with that, if you enable CONFIG_DEBUG_SECTION_MISMATCH in your kernel build, you might see what the compiler thinks is happening as well. I don't know if that tracks __ro_after_init usage, but it can't hurt. For this specific change, if you walk through the code paths by hand, you will see where the structure is modified when the driver is bound to a device. As that can happen any time after init has happened (i.e. after the module is loaded), I don't think this change is safe at all. Remember, you can add a PCI device at any time to lots of systems, it is a hotpluggable bus, just like USB. thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] staging: sm750fb: Make variable read only after initialization. 2016-10-19 6:48 ` Greg KH @ 2016-10-20 23:23 ` Elise Lennion 0 siblings, 0 replies; 6+ messages in thread From: Elise Lennion @ 2016-10-20 23:23 UTC (permalink / raw) To: Greg KH; +Cc: julia.lawall, sudipm.mukherjee, teddy.wang, outreachy-kernel On Wed, Oct 19, 2016 at 08:48:40AM +0200, Greg KH wrote: > On Wed, Oct 19, 2016 at 08:00:15AM +0200, Julia Lawall wrote: > > > > > > On Tue, 18 Oct 2016, Elise Lennion wrote: > > > > > On Mon, Oct 17, 2016 at 09:44:04AM +0200, Greg KH wrote: > > > > On Mon, Oct 17, 2016 at 12:32:16AM -0200, Elise Lennion wrote: > > > > > Use __ro_after_init to protect variable, initialized in several steps, > > > > > from overwrite during runtime. > > > > > > > > > > Signed-off-by: Elise Lennion <elise.lennion@gmail.com> > > > > > --- > > > > > drivers/staging/sm750fb/sm750.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c > > > > > index 0663ec0..5c02ad8 100644 > > > > > --- a/drivers/staging/sm750fb/sm750.c > > > > > +++ b/drivers/staging/sm750fb/sm750.c > > > > > @@ -719,7 +719,7 @@ static int sm750fb_set_drv(struct lynxfb_par *par) > > > > > return ret; > > > > > } > > > > > > > > > > -static struct fb_ops lynxfb_ops = { > > > > > +static __ro_after_init struct fb_ops lynxfb_ops = { > > > > > .owner = THIS_MODULE, > > > > > .fb_check_var = lynxfb_ops_check_var, > > > > > .fb_set_par = lynxfb_ops_set_par, > > > > > > > > How did you "prove" or test this? I don't think it is correct to mark > > > > this code this way at all, but would be glad to reconsider the patch if > > > > you can prove me otherwise :) > > > > > > > > thanks, > > > > > > > > greg k-h > > > > > > I compiled the file, used 'size' to verify that the data segment size > > > was smaller, installed the module, removed and reloaded the module with > > > modprobe and used lsmod to ensure the driver was loaded. > > > > > > Actually I just compiled the module and checked the data segment size > > > before sending the patch, but now I did the whole procedure and > > > everything seems right :) > > > > > > But I didn't find a good reference about how __ro_after_init works, > > > only a few old patches that use it, so I don't really know if it can > > > be loaded and still not have the desired effect, make lynxfb_ops read > > > only after initialization. > > > > > > I put it in that position because that's where a const would be, but > > > I searched more patches and most of them use __ro_after_init after the > > > name of the variable. Is that what's strange in this patch? > > > > > > I'll make a v2 with __ro_after_init in the standard position in case > > > that's the problem. > > > > I think it would be helpful to give an overview of how the structure is > > actually used in practice. Eg, it is only modified in function XX which > > is declared as __init, and otherwise it is only passed to function YY > > where the corresponding parameter is declared as const. Checking that the > > declaration is really compiled is good, but it doesn't check that all the > > uses are compiled in your configuration, so it is helpful to check the > > problem from multiple perspectives. > > Along with that, if you enable CONFIG_DEBUG_SECTION_MISMATCH in your > kernel build, you might see what the compiler thinks is happening as > well. I don't know if that tracks __ro_after_init usage, but it can't > hurt. > > For this specific change, if you walk through the code paths by hand, > you will see where the structure is modified when the driver is bound to > a device. As that can happen any time after init has happened (i.e. > after the module is loaded), I don't think this change is safe at all. > Remember, you can add a PCI device at any time to lots of systems, it is > a hotpluggable bus, just like USB. > > thanks, > > greg k-h Oh, now I see it isn't safe. The struct has attributes changed during the execution of probe(), so it can't be __ro_after_init. As a side note, I enabled CONFIG_DEBUG_SECTION_MISMATCH to see if it tracks __ro_after_init and judging by the output, I think it doesn't. thank you, elise ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-20 23:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-17 2:32 [PATCH] staging: sm750fb: Make variable read only after initialization Elise Lennion 2016-10-17 7:44 ` Greg KH 2016-10-18 21:46 ` Elise Lennion 2016-10-19 6:00 ` [Outreachy kernel] " Julia Lawall 2016-10-19 6:48 ` Greg KH 2016-10-20 23:23 ` Elise Lennion
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.