[RFC,v1,1/1] linux: mm_types: allow to modified the vm_flags in vm_area_struct

  With 6.3-rc1 is not possible to build the following drivers

- nvidia, that the compilation returns the following errors

/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c: In function ‘nvidia_mmap_numa’:
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:455:19: error: assignment of read-only member ‘vm_flags’
  455 |     vma->vm_flags |= VM_MIXEDMAP;
      |                   ^~
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c: In function ‘nvidia_mmap_helper’:
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:599:23: error: assignment of read-only member ‘vm_flags’
  599 |         vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND;
      |                       ^~
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:666:23: error: assignment of read-only member ‘vm_flags’
  666 |         vma->vm_flags |= (VM_IO | VM_LOCKED | VM_RESERVED);
      |                       ^~
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:667:23: error: assignment of read-only member ‘vm_flags’
  667 |         vma->vm_flags |= (VM_DONTEXPAND | VM_DONTDUMP);
      |                       ^~
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:673:23: error: assignment of read-only member ‘vm_flags’
  673 |         vma->vm_flags &= ~VM_WRITE;
      |                       ^~
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:674:23: error: assignment of read-only member ‘vm_flags’
  674 |         vma->vm_flags &= ~VM_MAYWRITE;
      |                       ^~
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c: In function ‘nv_encode_caching’:
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:353:16: warning: this statement may fall through [-Wimplicit-fallthrough=]
  353 |             if (NV_ALLOW_CACHING(memory_type))
      |                ^
/var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.c:356:9: note: here
  356 |         default:
      |         ^~~~~~~
make[2]: *** [scripts/Makefile.build:252: /var/lib/dkms/nvidia/525.89.02/build/nvidia/nv-mmap.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile:2028: /var/lib/dkms/nvidia/525.89.02/build] Error 2
make[1]: Leaving directory '/home/vincenzopalazzo/gittea/linux'
make: *** [Makefile:82: modules] Error 2
------

- vboxhost/7.0.6_OSE where the build fails with the following errors
DKMS make.log for vboxhost-7.0.6_OSE for kernel 6.3.0-rc1-00231-g511e7733223b (x86_64)
Sat Mar 11 04:21:34 PM UTC 2023
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetflt/linux/VBoxNetFlt-linux.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetadp/linux/VBoxNetAdp-linux.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetflt/VBoxNetFlt.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/linux/SUPDrv-linux.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/SUPDrv.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetadp/VBoxNetAdp.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/SUPDrvGip.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetflt/SUPR0IdcClient.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/SUPDrvSem.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetflt/SUPR0IdcClientComponent.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetflt/linux/SUPR0IdcClient-linux.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/SUPDrvTracer.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/SUPLibAll.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/common/string/strformatrt.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/combined-agnostic1.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/combined-agnostic2.o
  CC [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/combined-os-specific.o
  LD [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetadp/vboxnetadp.o
In file included from /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/combined-os-specific.c:43:
/var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/r0drv/linux/memobj-r0drv-linux.c: In function ‘rtR0MemObjNativeLockUser’:
/var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/r0drv/linux/memobj-r0drv-linux.c:1404:39: error: assignment of read-only member ‘vm_flags’
 1404 |                 papVMAs[rc]->vm_flags |= VM_DONTCOPY | VM_LOCKED;
      |                                       ^~
/var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/r0drv/linux/memobj-r0drv-linux.c: In function ‘rtR0MemObjNativeMapUser’:
/var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/r0drv/linux/memobj-r0drv-linux.c:1876:35: error: assignment of read-only member ‘vm_flags’
 1876 |                     vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
      |                                   ^~
make[2]: *** [scripts/Makefile.build:252: /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv/combined-os-specific.o] Error 1
make[2]: *** Waiting for unfinished jobs....
  LD [M]  /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxnetflt/vboxnetflt.o
make[1]: *** [scripts/Makefile.build:494: /var/lib/dkms/vboxhost/7.0.6_OSE/build/vboxdrv] Error 2
make: *** [Makefile:2028: /var/lib/dkms/vboxhost/7.0.6_OSE/build] Error 2
-----

The commands that I use to build these drivers are:

- sudo dkms install --no-depmod nvidia/525.89.02 -k 6.3.0-rc1-00231-g511e7733223b
- sudo dkms install --no-depmod vboxhost/7.0.6_OSE -k 6.3.0-rc1-00231-g511e7733223b

The following commit reverts the const constraint that was added in [1],
and also I tested this patch on my system and all started again as
6.2.2.

[1] https://lore.kernel.org/all/20230126193752.297968-3-surenb@google.com/T/#u

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
---
 include/linux/mm_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Sun, 12 Mar 2023 23:42:50 +0100 Vincenzo Palazzo <vincenzopalazzodev@gmail.com> wrote:

> With 6.3-rc1 is not possible to build the following drivers

Well, let's cc Suren who actually wrote bc292ab00f6c ("mm: introduce
vma->vm_flags wrapper functions").

> - nvidia, that the compilation returns the following errors
> 
> - vboxhost/7.0.6_OSE where the build fails with the following errors
> 
> ...
>
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -497,7 +497,7 @@ struct vm_area_struct {
>  	 * To modify use vm_flags_{init|reset|set|clear|mod} functions.
>  	 */
>  	union {
> -		const vm_flags_t vm_flags;
> +		vm_flags_t vm_flags;
>  		vm_flags_t __private __vm_flags;
>  	};

These out-of-tree drivers should be converted to use the wrappers which
bc292ab00f6c added.  Their maintainers have a couple of months to do
this before bc292ab00f6c is released in 6.3.

It's unfortunate.  I guess it would be better if we were to find a way
to permit these drivers to compile OK but to generate warnings, as an
interim thing for six months or so.  Perhaps there's some clever way.
  

On Sun, Mar 12, 2023 at 11:42:50PM +0100, Vincenzo Palazzo wrote:
> With 6.3-rc1 is not possible to build the following drivers
> 
> - nvidia, that the compilation returns the following errors

<snip>

> - vboxhost/7.0.6_OSE where the build fails with the following errors

Out-of-tree code needs to be fixed up all the time, this is their
responsibility, not the kernel's responsibility.  Please submit changes
to their codebases if you rely on these modules.

thanks,

greg k-h
  

On Mon Mar 13, 2023 at 1:57 AM CET, Andrew Morton wrote:
> On Sun, 12 Mar 2023 23:42:50 +0100 Vincenzo Palazzo <vincenzopalazzodev@gmail.com> wrote:
>
> > With 6.3-rc1 is not possible to build the following drivers
>
> Well, let's cc Suren who actually wrote bc292ab00f6c ("mm: introduce
> vma->vm_flags wrapper functions").
>
> > - nvidia, that the compilation returns the following errors
> > 
> > - vboxhost/7.0.6_OSE where the build fails with the following errors
> > 
> > ...
> >
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -497,7 +497,7 @@ struct vm_area_struct {
> >  	 * To modify use vm_flags_{init|reset|set|clear|mod} functions.
> >  	 */
> >  	union {
> > -		const vm_flags_t vm_flags;
> > +		vm_flags_t vm_flags;
> >  		vm_flags_t __private __vm_flags;
> >  	};
>
> These out-of-tree drivers should be converted to use the wrappers which
> bc292ab00f6c added.  Their maintainers have a couple of months to do
> this before bc292ab00f6c is released in 6.3.

Yeah I thought about that! nvidia update as soon as the 6.3 is out, not sure
about vboxhost. But let the driver update to this new feature make sense.

>
> It's unfortunate.  I guess it would be better if we were to find a way
> to permit these drivers to compile OK but to generate warnings, as an
> interim thing for six months or so.  Perhaps there's some clever way.

Yeah it is! Regarding giving the possiblity to compile ok (and
preserving the previous behavior) look like an interesting thought for 
kernel API, but am I think that it this is difficult to do?

The real things that I'm missing with the new wrapper API
is the meaning of the new `vm_flags_t vm_flags`, before the 
field was only one and it was getting modified, but now the real
value is the private one `__vm_flags`, so what is the meaning of 
the new `vm_flags`?

I feel that this question is stupid but I can not avoid do it, otherwise 
my mind think about it till I will not find an answer :)

P.S: regaring the warning at compile time the __deprecated looks a good
fit to generate error message, but to me in this particular case do not.

Thanks,

Vincent.
  

On Mon, Mar 13, 2023 at 10:25:19AM +0100, Vincenzo Palazzo wrote:
> P.S: regaring the warning at compile time the __deprecated looks a good
> fit to generate error message, but to me in this particular case do not.

__deprecated does not work in the kernel tree, sorry.  We have tried
that in the past and it just gets ignored.  Instead, we fix up all
in-kernel users and continue on.

thanks,

greg k-h
  

On Mon, Mar 13, 2023 at 2:25 AM Vincenzo Palazzo
<vincenzopalazzodev@gmail.com> wrote:
>
> On Mon Mar 13, 2023 at 1:57 AM CET, Andrew Morton wrote:
> > On Sun, 12 Mar 2023 23:42:50 +0100 Vincenzo Palazzo <vincenzopalazzodev@gmail.com> wrote:
> >
> > > With 6.3-rc1 is not possible to build the following drivers
> >
> > Well, let's cc Suren who actually wrote bc292ab00f6c ("mm: introduce
> > vma->vm_flags wrapper functions").
> >
> > > - nvidia, that the compilation returns the following errors
> > >
> > > - vboxhost/7.0.6_OSE where the build fails with the following errors
> > >
> > > ...
> > >
> > > --- a/include/linux/mm_types.h
> > > +++ b/include/linux/mm_types.h
> > > @@ -497,7 +497,7 @@ struct vm_area_struct {
> > >      * To modify use vm_flags_{init|reset|set|clear|mod} functions.
> > >      */
> > >     union {
> > > -           const vm_flags_t vm_flags;
> > > +           vm_flags_t vm_flags;
> > >             vm_flags_t __private __vm_flags;
> > >     };
> >
> > These out-of-tree drivers should be converted to use the wrappers which
> > bc292ab00f6c added.  Their maintainers have a couple of months to do
> > this before bc292ab00f6c is released in 6.3.
>
> Yeah I thought about that! nvidia update as soon as the 6.3 is out, not sure
> about vboxhost. But let the driver update to this new feature make sense.
>
> >
> > It's unfortunate.  I guess it would be better if we were to find a way
> > to permit these drivers to compile OK but to generate warnings, as an
> > interim thing for six months or so.  Perhaps there's some clever way.
>
> Yeah it is! Regarding giving the possiblity to compile ok (and
> preserving the previous behavior) look like an interesting thought for
> kernel API, but am I think that it this is difficult to do?
>
> The real things that I'm missing with the new wrapper API
> is the meaning of the new `vm_flags_t vm_flags`, before the
> field was only one and it was getting modified, but now the real
> value is the private one `__vm_flags`, so what is the meaning of
> the new `vm_flags`?

Sorry for the delay. I'm on vacation until Wednesday and have limited
network access.
vm_flags is used as a read-only property so that readers of the flags
don't have to change, however flag modifiers should be using new
wrapper functions. Fixing the errors in this report should be trivial.
For example:
vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND;
should be replaced with
vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND);
and
vma->vm_flags &= ~VM_MAYWRITE;
with
vm_flags_clear(vma, VM_MAYWRITE);

Thanks,
Suren.

>
> I feel that this question is stupid but I can not avoid do it, otherwise
> my mind think about it till I will not find an answer :)
>
> P.S: regaring the warning at compile time the __deprecated looks a good
> fit to generate error message, but to me in this particular case do not.
>
> Thanks,
>
> Vincent.
>