* [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.