diff mbox series

[v4,8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'

Message ID 20210120173612.20913-9-will@kernel.org (mailing list archive)
State New
Headers show
Series Create 'old' ptes for faultaround mappings on arm64 with hardware access flag | expand

Commit Message

Will Deacon Jan. 20, 2021, 5:36 p.m. UTC
The fields of this struct are only ever read after being initialised, so
mark it 'const' before somebody tries to modify it again.

Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 include/linux/mm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nick Desaulniers Jan. 20, 2021, 6:27 p.m. UTC | #1
On Wed, Jan 20, 2021 at 9:36 AM Will Deacon <will@kernel.org> wrote:
>
> The fields of this struct are only ever read after being initialised, so
> mark it 'const' before somebody tries to modify it again.
>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  include/linux/mm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e0f056753bef..7ff3d9817d38 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -514,7 +514,7 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags)
>   * pgoff should be used in favour of virtual_address, if possible.
>   */
>  struct vm_fault {
> -       struct {
> +       const struct {
>                 struct vm_area_struct *vma;     /* Target VMA */
>                 gfp_t gfp_mask;                 /* gfp mask to be used for allocations */
>                 pgoff_t pgoff;                  /* Logical page offset based on vma */

Is there a difference between:

+       const struct {
+               struct vm_area_struct *vma;     /* Target VMA */
+               gfp_t gfp_mask;                 /* gfp mask to be used
for allocations */
+               pgoff_t pgoff;                  /* Logical page offset
based on vma */
+               unsigned long address;          /* Faulting virtual address */
+       };

vs

+       struct {
+               struct vm_area_struct * const vma;     /* Target VMA */
+               const gfp_t gfp_mask;                 /* gfp mask to
be used for allocations */
+               const pgoff_t pgoff;                  /* Logical page
offset based on vma */
+               const unsigned long address;          /* Faulting
virtual address */
+       };

ie. marking the members const vs the anonymous struct?  Does anything
need to change in the use of these structures?

> --
> 2.30.0.284.gd98b1dd5eaa7-goog
>
Linus Torvalds Jan. 20, 2021, 7:02 p.m. UTC | #2
On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Is there a difference between: [ const unnamed struct and individual const members ]

Semantically? No.

Syntactically the "group the const members together" is a lot cleaner,
imho. Not just from a "just a single const" standpoint, but from a
"code as documentation" standpoint.

But I guess to avoid the clang issue, we could do the "mark individual
fields" thing.

(It turns out that sparse gets this wrong too, so it's not just clang).

           Linus
Will Deacon Jan. 21, 2021, 1:11 p.m. UTC | #3
On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote:
> On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Is there a difference between: [ const unnamed struct and individual const members ]
> 
> Semantically? No.
> 
> Syntactically the "group the const members together" is a lot cleaner,
> imho. Not just from a "just a single const" standpoint, but from a
> "code as documentation" standpoint.
> 
> But I guess to avoid the clang issue, we could do the "mark individual
> fields" thing.

I'd prefer to wait until the bug against LLVM has been resolved before we
try to work around anything. Although I couldn't find any other examples
like this in the kernel, requiring all of the member fields to be marked as
'const' still feels pretty fragile to me; it's only a matter of time before
new non-const fields get added, at which point the temptation for developers
to remove 'const' from other fields when it gets in the way is pretty high.

None of this is bullet-proof, of course, but if clang ends up emitting a
warning (even if it's gated behind an option) then I think we're in a good
place.

> (It turns out that sparse gets this wrong too, so it's not just clang).

Adding Luc, as hopefully that's fixable.

Will
Nick Desaulniers Jan. 21, 2021, 7:24 p.m. UTC | #4
On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote:
>
> On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote:
> > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > Is there a difference between: [ const unnamed struct and individual const members ]
> >
> > Semantically? No.
> >
> > Syntactically the "group the const members together" is a lot cleaner,
> > imho. Not just from a "just a single const" standpoint, but from a
> > "code as documentation" standpoint.
> >
> > But I guess to avoid the clang issue, we could do the "mark individual
> > fields" thing.
>
> I'd prefer to wait until the bug against LLVM has been resolved before we
> try to work around anything. Although I couldn't find any other examples
> like this in the kernel, requiring all of the member fields to be marked as
> 'const' still feels pretty fragile to me; it's only a matter of time before
> new non-const fields get added, at which point the temptation for developers
> to remove 'const' from other fields when it gets in the way is pretty high.

What's to stop a new non-const field from getting added outside the
const qualified anonymous struct?
What's to stop someone from removing const from the anonymous struct?
What's to stop a number of callers from manipulating the structure
temporarily before restoring it when returning by casting away the
const?

Code review.

Using a non-toolchain-portable solution certainly could be considered
more fragile.

It's always possible that the resolution is the C standards body goes
the C++ route, at which point GCC would be forced to address this and
potentially change behavior.  Kind of like how people avoid going to
court since things are never guaranteed to work out in their favor.

>
> None of this is bullet-proof, of course, but if clang ends up emitting a
> warning (even if it's gated behind an option) then I think we're in a good
> place.
>
> > (It turns out that sparse gets this wrong too, so it's not just clang).
>
> Adding Luc, as hopefully that's fixable.
>
> Will



--
Thanks,
~Nick Desaulniers
Will Deacon Jan. 21, 2021, 9:28 p.m. UTC | #5
On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote:
> On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote:
> > > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers
> > > <ndesaulniers@google.com> wrote:
> > > >
> > > > Is there a difference between: [ const unnamed struct and individual const members ]
> > >
> > > Semantically? No.
> > >
> > > Syntactically the "group the const members together" is a lot cleaner,
> > > imho. Not just from a "just a single const" standpoint, but from a
> > > "code as documentation" standpoint.
> > >
> > > But I guess to avoid the clang issue, we could do the "mark individual
> > > fields" thing.
> >
> > I'd prefer to wait until the bug against LLVM has been resolved before we
> > try to work around anything. Although I couldn't find any other examples
> > like this in the kernel, requiring all of the member fields to be marked as
> > 'const' still feels pretty fragile to me; it's only a matter of time before
> > new non-const fields get added, at which point the temptation for developers
> > to remove 'const' from other fields when it gets in the way is pretty high.
> 
> What's to stop a new non-const field from getting added outside the
> const qualified anonymous struct?
> What's to stop someone from removing const from the anonymous struct?
> What's to stop a number of callers from manipulating the structure
> temporarily before restoring it when returning by casting away the
> const?
> 
> Code review.

Sure, but here we are cleaning up this stuff, so I think review only gets
you so far. To me:

	const struct {
		int	foo;
		long	bar;
	};

clearly says "don't modify fields of this struct", whereas:

	struct {
		const int	foo;
		const long	bar;
	};

says "don't modify foo or bar, but add whatever you like on the end" and
that's the slippery slope. So then we end up with the eye-sore of:

	const struct {
		const int	foo;
		const long	bar;
	};

and maybe that's the right answer, but I'm just saying we should wait for
clang to make up its mind first. It's not like this is a functional problem,
and there are enough GCC users around that we're not exactly in a hurry.

Will
Linus Torvalds Jan. 22, 2021, 5:47 p.m. UTC | #6
On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote:
>
> > (It turns out that sparse gets this wrong too, so it's not just clang).
>
> Adding Luc, as hopefully that's fixable.

I had talked to Luc about this earlier, and he just sent out a fix for
sparse. It's not in the git repo yet, but it's coming.

Having looked at what sparse does, I suspect the clang behavior stems
from a similar approach to looking up unnamed struct/union members.

And the sparse fix was fairly straightforward: changing the _lookup_
is painful, because that's late and trying to fix types after-the-fact
is just not great. But just (recursively) modifying the type modifiers
at type parsing time for anonymous struct/union members is quite
straightforward, probably in clang too.

So honestly - I think the clang push-back was by somebody who thought
it would be nasty to fix, and who was thus actively trying to mis-read
the standards language.

I'm not willing to argue with compiler people who do standards
language weasel-wording (I've seen it before, my life is too short to
deal with those kinds of people), but maybe Nick is more used to deal
with clang people.

Nick - I suspect that the sparse type system model is different enough
from the clang one that the sparse patch is not really helpful as a
"look, this is how it was done in sparse, maybe clang can do something
similar", but I'll bounce it to you separately anyway just in case.
Maybe your clang knowledge means that you go "yeah, in clang that
function is called 'xyz', and we can do something very similar".

            Linus
Nick Desaulniers Jan. 22, 2021, 7:10 p.m. UTC | #7
On Thu, Jan 21, 2021 at 1:28 PM Will Deacon <will@kernel.org> wrote:
>
> On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote:
> > On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote:
> > > > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers
> > > > <ndesaulniers@google.com> wrote:
> > > > >
> > > > > Is there a difference between: [ const unnamed struct and individual const members ]
> > > >
> > > > Semantically? No.
> > > >
> > > > Syntactically the "group the const members together" is a lot cleaner,
> > > > imho. Not just from a "just a single const" standpoint, but from a
> > > > "code as documentation" standpoint.
> > > >
> > > > But I guess to avoid the clang issue, we could do the "mark individual
> > > > fields" thing.
> > >
> > > I'd prefer to wait until the bug against LLVM has been resolved before we
> > > try to work around anything. Although I couldn't find any other examples
> > > like this in the kernel, requiring all of the member fields to be marked as
> > > 'const' still feels pretty fragile to me; it's only a matter of time before
> > > new non-const fields get added, at which point the temptation for developers
> > > to remove 'const' from other fields when it gets in the way is pretty high.
> >
> > What's to stop a new non-const field from getting added outside the
> > const qualified anonymous struct?
> > What's to stop someone from removing const from the anonymous struct?
> > What's to stop a number of callers from manipulating the structure
> > temporarily before restoring it when returning by casting away the
> > const?
> >
> > Code review.
>
> Sure, but here we are cleaning up this stuff, so I think review only gets
> you so far. To me:
>
>         const struct {
>                 int     foo;
>                 long    bar;
>         };
>
> clearly says "don't modify fields of this struct", whereas:
>
>         struct {
>                 const int       foo;
>                 const long      bar;
>         };
>
> says "don't modify foo or bar, but add whatever you like on the end" and
> that's the slippery slope.

"but you could add additional non-const members on the end" for sure.
Though going back to

>> What's to stop a new non-const field from getting added outside the
> > const qualified anonymous struct?

my point with that is that the const anonymous struct is within a
non-const anonymous struct.

struct vm_fault {
  const {
    struct vm_area_struct *vma;
    gfp_t gfp_mask;
    pgoff_t pgoff;
    unsigned long address;
    // Your point is about new member additions here, IIUC
  };
  // My point: what's to stop someone from adding a new non-const member here?
  unsigned int flags;
  pmd_t *pmd;
  pud_t *pud;
  ...
  // or here?
};

The const anonymous struct will help prevent additions of non-const
members to the anonymous struct, sure; but if someone really wanted a
new non-const member in a `struct vm_fault` instance they're just
going to go around the const anonymous struct.  Or is there something
more I'm missing about the order of the members of struct vm_fault?

> So then we end up with the eye-sore of:
>
>         const struct {
>                 const int       foo;
>                 const long      bar;
>         };
>
> and maybe that's the right answer, but I'm just saying we should wait for
> clang to make up its mind first. It's not like this is a functional problem,
> and there are enough GCC users around that we're not exactly in a hurry.

Yeah, I mean I'm happy to whip something up for Clang, even though I
suspect it will get shot down in code review until there's more
guidance from standards bodies.  It doesn't hurt to try, and at least
have a patch "waiting in the wings" should we hear back from WG14 that
favors GCC's behavior.  Who knows, maybe the guidance will be that
WG21 should revisit this behavior for C++ to avoid divergence with C
(as g++ and gcc currently do).
Will Deacon Jan. 22, 2021, 7:27 p.m. UTC | #8
Hey Nick,

On Fri, Jan 22, 2021 at 11:10:50AM -0800, Nick Desaulniers wrote:
> On Thu, Jan 21, 2021 at 1:28 PM Will Deacon <will@kernel.org> wrote:
> > On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote:
> > > On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote:
> > Sure, but here we are cleaning up this stuff, so I think review only gets
> > you so far. To me:
> >
> >         const struct {
> >                 int     foo;
> >                 long    bar;
> >         };
> >
> > clearly says "don't modify fields of this struct", whereas:
> >
> >         struct {
> >                 const int       foo;
> >                 const long      bar;
> >         };
> >
> > says "don't modify foo or bar, but add whatever you like on the end" and
> > that's the slippery slope.
> 
> "but you could add additional non-const members on the end" for sure.
> Though going back to
> 
> >> What's to stop a new non-const field from getting added outside the
> > > const qualified anonymous struct?
> 
> my point with that is that the const anonymous struct is within a
> non-const anonymous struct.
> 
> struct vm_fault {
>   const {
>     struct vm_area_struct *vma;
>     gfp_t gfp_mask;
>     pgoff_t pgoff;
>     unsigned long address;
>     // Your point is about new member additions here, IIUC
>   };
>   // My point: what's to stop someone from adding a new non-const member here?
>   unsigned int flags;
>   pmd_t *pmd;
>   pud_t *pud;
>   ...
>   // or here?
> };
> 
> The const anonymous struct will help prevent additions of non-const
> members to the anonymous struct, sure; but if someone really wanted a
> new non-const member in a `struct vm_fault` instance they're just
> going to go around the const anonymous struct.  Or is there something
> more I'm missing about the order of the members of struct vm_fault?

All I was trying to say is that, given a struct with a mixture of const and
non-const members, I would be less hesitant to remove 'const' from one of
the members if it helped me get something else done. Having the 'const' on
the struct itself, however, would deter me because at that point its clear
that you're not supposed to be modifying this stuff. That's the difference
between the "Your point" and "My point" lines above.

But honestly, I can't say I particularly enjoy arguing about these
idiosyncracies; I'd just rather wait for the dust to settle with clang
before we add code to deal with that outcome.

> > So then we end up with the eye-sore of:
> >
> >         const struct {
> >                 const int       foo;
> >                 const long      bar;
> >         };
> >
> > and maybe that's the right answer, but I'm just saying we should wait for
> > clang to make up its mind first. It's not like this is a functional problem,
> > and there are enough GCC users around that we're not exactly in a hurry.
> 
> Yeah, I mean I'm happy to whip something up for Clang, even though I
> suspect it will get shot down in code review until there's more
> guidance from standards bodies.  It doesn't hurt to try, and at least
> have a patch "waiting in the wings" should we hear back from WG14 that
> favors GCC's behavior.  Who knows, maybe the guidance will be that
> WG21 should revisit this behavior for C++ to avoid divergence with C
> (as g++ and gcc currently do).

I mean, a warning doesn't seem controversial to me, especially as opinions
certainly seem to be divided about what the right behaviour should be here.

Will
diff mbox series

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e0f056753bef..7ff3d9817d38 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -514,7 +514,7 @@  static inline bool fault_flag_allow_retry_first(unsigned int flags)
  * pgoff should be used in favour of virtual_address, if possible.
  */
 struct vm_fault {
-	struct {
+	const struct {
 		struct vm_area_struct *vma;	/* Target VMA */
 		gfp_t gfp_mask;			/* gfp mask to be used for allocations */
 		pgoff_t pgoff;			/* Logical page offset based on vma */