Received-SPF: pass (google.com: domain of linux-kernel+bounces-166866-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <CANpmjNN250UCxsWCpUHAvJo28Lzv=DN-BKTmjEpcLFOCA+U+pw@mail.gmail.com>
 <CAHk-=whnQXNVwuf42Sh2ngBGhBqbJjUfq5ux6e7Si_XSPAt05A@mail.gmail.com>
 <d4de136e-c4e0-45c2-b33e-9a819cb3a791@paulmck-laptop> <CAHk-=wi3iondeh_9V2g3Qz5oHTRjLsOpoy83hb58MVh=nRZe0A@mail.gmail.com>
 <892324fc-9b75-4e8a-b3b6-cf3c5b4c3506@paulmck-laptop> <CANpmjNOY=Qpm3hBu-eN4Xk8n-2VXQRvcQ3_PfwPwNw9MmC8ctw@mail.gmail.com>
 <CAHk-=whTakjVGgBC5OtoZ5Foo=hd4-g+NZ79nkMDVj6Ug7ARKQ@mail.gmail.com>
 <CANpmjNNo_jyTPrgPVCeSfgvsX-fK8x0H81zbBA6LZMVNodO6GA@mail.gmail.com>
 <b13ab60e-503a-4c11-8a99-0ccccce33c6c@I-love.SAKURA.ne.jp>
 <CAHk-=wi_QBG68QshO1e-xK-jt0ZFsMpZczEJe4nQMu+U7q_7EQ@mail.gmail.com> <20240502181444.GF2118490@ZenIV>
In-Reply-To: <20240502181444.GF2118490@ZenIV>
From: Marco Elver <elver@google.com>
Date: Thu, 2 May 2024 21:29:04 +0200
Message-ID: <CANpmjNN0n9UkYheZyBCQykrLYM9EDsmkp41a=x4hbgYyKDPZxw@mail.gmail.com>
Subject: Re: [PATCH v3] tty: tty_io: remove hung_up_tty_fops
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>, 
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>, paulmck@kernel.org, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Dmitry Vyukov <dvyukov@google.com>, 
	syzbot <syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com>, 
	linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, 
	Nathan Chancellor <nathan@kernel.org>, Arnd Bergmann <arnd@kernel.org>, Jiri Slaby <jirislaby@kernel.org>
Content-Type: text/plain; charset="UTF-8"

On Thu, 2 May 2024 at 20:14, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, May 02, 2024 at 10:29:52AM -0700, Linus Torvalds wrote:
>
> > Yes, this is unusual. The *common* thing is to mark pointers as being
> > volatile. But this really is something entirely different from that.
>
> The common thing is to mark pointers are pointers to volatile;
> calling them "volatile pointers" is common and incorrect, and the only
> reason why that sloppy turn of phrase persists is that real "volatile
> pointers" are rare...
>
> Marco,

I think we agree on what we want. I misread the intention of Tetsuo in
[1], and provided incorrect feedback.

[1] https://lore.kernel.org/all/CANpmjNPtoKf1ysbKd=E8o753JT0DzBanzFBP234VBsazfufVAQ@mail.gmail.com/T/#u

>         struct foo volatile *p;
> declares p as a (non-volatile) pointer to volatile struct foo.
>         struct foo * volatile p;
> declares p as volatile pointer to (non-volatile) struct foo.
>
> The former is a statement about the objects whose addresses might
> be stored in p; the latter is a statement about the object p itself.
>
> Replace volatile with const and it becomes easier to experiment with:
>         char const *p;
>         char s[] = "barf";
>         char * const q = s;
>         ...
>         p = "yuck";     - fine, p itself can be modified
>         *p = 'a';       - error *p can not be modified, it's an l-value of type const char
>         q = s + 1;      - error, can't modify q
>         *q = 'a';       - fine, *q is l-value of type char
>         p = q;          - fine, right-hand side of assignment loses the top
>                           qualifier, so q (const pointer to char as l-value)
>                           becomes a plain pointer to char, which can be
>                           converted to pointer to const char, and stored in
>                           p (l-value of type pointer to const char)
>         strlen(q);      - almost the same story, except that it's passing
>                           an argument rather than assignment; they act the
>                           same way.
>         strcpy(q, "s"); - almost the same, except that here the type of
>                           argument is pointer to char rather than pointer to
>                           const char (strlen() promises not to modify the
>                           string passed to it, strcpy() obviously doesn't)
>         strcpy(p, "s"); - error; pointer to char converts to a pointer
>                           to const char, but not the other way round.
>
> The situations where you want a const (or volatile) pointer (as opposed to
> pointer to const or volatile object) are rare, but this is exactly what
> you are asking for - you want to say that the value of 'f_op' member
> in any struct file can change at any time.  That value is an address of
> some instance of struct file_operations and what you want to express is
> the property of f_op member itself, not that of the objects whose addresses
> might end up stored there.
>
> So having a driver do
>         const struct file_operations *ops = file->f_op;
> is fine - it's basically "take the value of 'file'; it will be an address
> of some struct file instance.  Fetch 'f_op' from that instance, without
> any assumptions of the stability of that member.  Use whatever value
> you find there as initial value of 'ops'".
>
> That's fine, and since nobody is going to change 'ops' itself behind your
> back, you don't need any qualifiers on it.  The type of 'ops' here is
> "(unqualified) pointer to const struct file_operations".

Is this feedback for the __data_racy attribute patch [2], or a comment
for the patch "tty: tty_io: remove hung_up_tty_fops"? [ With the
former I can help, with the latter Tetsuo can help. ]

[2] https://lore.kernel.org/all/20240502141242.2765090-1-elver@google.com/

The __data_racy attribute should behave like any other type qualifier
(be it const, volatile), and what you point out above applies equally,
no doubt about it. But I think it's important to treat it as a
completely distinct type qualifier - volatile is an implementation
detail (in non-KCSAN kernels it's a no-op).

Thanks,
-- Marco