Re: [Outreachy kernel] Re: [PATCH] staging: sm750fb: Make variable read only after initialization.

[Elise Lennion <elise.lennion@gmail.com>]

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