staging: sm750fb: Make variable read only after initialization.

Use __ro_after_init to protect variable, initialized in several steps, from overwrite during runtime.

Signed-off-by Elise Lennion <[email protected]>

---
 drivers/staging/sm750fb/sm750.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/sm750fb/sm750.c +0 -0

index 0663ec0..5c02ad8 100644

Quoted 13 lines · Elise Lennion

Use __ro_after_init to protect variable, initialized in several steps, from overwrite during runtime.

Signed-off-by Elise Lennion <[email protected]>

---
 drivers/staging/sm750fb/sm750.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/sm750fb/sm750.c +0 -0

index 0663ec0..5c02ad8 100644

drivers/staging/sm750fb/sm750.c static int sm750fb_set_drv(struct lynxfb_par *par) · line 725

Quoted 8 lines

drivers/staging/sm750fb/sm750.c +1 -1

@@ -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

Quoted 27 lines · Greg KH

Quoted 13 lines · Elise Lennion

Use __ro_after_init to protect variable, initialized in several steps, from overwrite during runtime.

Signed-off-by Elise Lennion <[email protected]>

---
 drivers/staging/sm750fb/sm750.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/sm750fb/sm750.c +0 -0

index 0663ec0..5c02ad8 100644

drivers/staging/sm750fb/sm750.c static int sm750fb_set_drv(struct lynxfb_par *par) · line 725

Quoted 8 lines

drivers/staging/sm750fb/sm750.c +1 -1

@@ -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 :)

Quoted 3 lines

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.

Quoted 45 lines · Elise Lennion

Quoted 27 lines · Greg KH

Quoted 13 lines · Elise Lennion

Use __ro_after_init to protect variable, initialized in several steps, from overwrite during runtime.

Signed-off-by Elise Lennion <[email protected]>

---
 drivers/staging/sm750fb/sm750.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/sm750fb/sm750.c +0 -0

index 0663ec0..5c02ad8 100644

drivers/staging/sm750fb/sm750.c static int sm750fb_set_drv(struct lynxfb_par *par) · line 725

Quoted 8 lines

drivers/staging/sm750fb/sm750.c +1 -1

@@ -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 :)

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 56 lines · Julia Lawall

Quoted 45 lines · Elise Lennion

Quoted 27 lines · Greg KH

Quoted 13 lines · Elise Lennion

Use __ro_after_init to protect variable, initialized in several steps, from overwrite during runtime.

Signed-off-by Elise Lennion <[email protected]>

---
 drivers/staging/sm750fb/sm750.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/sm750fb/sm750.c +0 -0

index 0663ec0..5c02ad8 100644

drivers/staging/sm750fb/sm750.c static int sm750fb_set_drv(struct lynxfb_par *par) · line 725

Quoted 8 lines

drivers/staging/sm750fb/sm750.c +1 -1

@@ -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 :)

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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