Re: [PATCH] Staging : android: Struct file_operations should be const

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Feb 07, 2022 at 04:01:01PM +0000, Al Viro wrote:
> On Mon, Feb 07, 2022 at 12:17:11AM -0300, Leonardo Araujo wrote:
> > From: "Leonardo Araujo" <leonardo.aa88@gmail.com>
> > 
> > WARNING: struct file_operations should normally be const
> > 
> > Signed-off-by: Leonardo Araujo <leonardo.aa88@gmail.com>
> > ---
> >  drivers/staging/android/ashmem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> > index ddbde3f8430e..4c6b420fbf4d 100644
> > --- a/drivers/staging/android/ashmem.c
> > +++ b/drivers/staging/android/ashmem.c
> > @@ -377,7 +377,7 @@ ashmem_vmfile_get_unmapped_area(struct file *file, unsigned long addr,
> >  
> >  static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> >  {
> > -	static struct file_operations vmfile_fops;
> > +	static const struct file_operations vmfile_fops;
> >  	struct ashmem_area *asma = file->private_data;
> >  	int ret = 0;
> 
> Wait a minute.  Why the hell would it possibly want a private instance
> of all-NULLs file_operations?  Odd...
> <checks>
>                 if (!vmfile_fops.mmap) {
>                         vmfile_fops = *vmfile->f_op;
>                         vmfile_fops.mmap = ashmem_vmfile_mmap;
>                         vmfile_fops.get_unmapped_area =
>                                         ashmem_vmfile_get_unmapped_area;
>                 }
> Er...  So it *is* modified down the road.  What, in your opinion, is signified
> by the const you are adding?
> 
> Folks, could we please have the first "WARNING" in checkpatch.pl output replaced
> with
> "I'm a dumb script; this line looks like there might be something fishy in the
> area.  Somebody smarter than me might want to take a look and figure out if
> there's something wrong going on there.  From now on I'll mark all such places
> with 'WARNING' (with the summary of heuristics that pointed to them), to avoid
> repeating the above".
> 
> Pretty please?  This exact trap keeps snagging newbies - folks misinterpret
> "this place might be worth looking into" for "great (s)tool says: this is
> what's wrong there; must propitiate the great (s)tool!"
> 
> In this case the damage is minimal - the resulting change would be instantly
> caught by compiler, so it's just a matter of mild embarrassment for poster.
> In other cases results had been nowhere near as mild.

It's a great "catch" for people who try to modify the kernel and then
never actually test-build their changes.  So for that reason alone I
like it as it does reinforce the need for drive-by people to at least
build the kernel tree.

> Incidentally, the place those heuristics had pointed too _DOES_ look fishy,
> indeed.  What happens, AFAICS, is that the first time we hit that branch
> (asma->file being NULL) we stash a copy of whatever file_operations we get
> on file obtained by shmem_setup_file() (IOW, shmem_file_operations),
> with ->mmap and ->get_unmapped_area replaced with local functions.
> This is a bloody convoluted way to do things, not to mention being rather
> brittle...
> 

Ashmem is horrid and is not used by the Android project anymore, except
by really old userspace programs.  I think we could just delete it
entirely now, I'll go ask the Android developers about it.

thanks,

greg k-h