From: Marco Elver <elver@google.com>
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>
Subject: Re: [PATCH v3] tty: tty_io: remove hung_up_tty_fops
Date: Thu, 2 May 2024 21:29:04 +0200 [thread overview]
Message-ID: <CANpmjNN0n9UkYheZyBCQykrLYM9EDsmkp41a=x4hbgYyKDPZxw@mail.gmail.com> (raw)
In-Reply-To: <20240502181444.GF2118490@ZenIV>
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
next prev parent reply other threads:[~2024-05-02 19:29 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-21 8:18 [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4) syzbot
2023-04-21 8:21 ` Dmitry Vyukov
2023-04-21 15:12 ` Tetsuo Handa
2023-04-21 16:02 ` Tetsuo Handa
2023-04-23 23:34 ` Al Viro
2023-04-23 23:55 ` Tetsuo Handa
2023-04-24 0:44 ` Al Viro
2023-04-24 1:09 ` Tetsuo Handa
2023-04-25 14:47 ` Tetsuo Handa
2023-04-25 16:03 ` Al Viro
2023-04-25 22:09 ` Tetsuo Handa
2023-04-26 11:05 ` [PATCH] tty: tty_io: remove hung_up_tty_fops Tetsuo Handa
2023-04-28 16:27 ` Nathan Chancellor
2023-04-28 16:41 ` Tetsuo Handa
2023-04-28 17:11 ` Al Viro
2023-04-29 10:43 ` Tetsuo Handa
2023-04-28 17:31 ` Greg Kroah-Hartman
2023-04-29 15:21 ` Guenter Roeck
2023-05-01 18:42 ` Geert Uytterhoeven
2023-05-14 1:02 ` [PATCH v2] " Tetsuo Handa
2023-05-30 10:44 ` Greg Kroah-Hartman
2023-05-30 11:57 ` Tetsuo Handa
2023-05-30 12:51 ` Greg Kroah-Hartman
2024-04-27 6:20 ` [PATCH v3] " Tetsuo Handa
2024-04-27 19:02 ` Linus Torvalds
2024-04-28 10:19 ` Tetsuo Handa
2024-04-28 18:50 ` Linus Torvalds
2024-04-29 13:55 ` Marco Elver
2024-04-29 15:38 ` Linus Torvalds
2024-05-01 18:45 ` Paul E. McKenney
2024-05-01 18:56 ` Linus Torvalds
2024-05-01 19:02 ` Paul E. McKenney
2024-05-01 20:14 ` Marco Elver
2024-05-01 21:06 ` Linus Torvalds
2024-05-01 21:20 ` Linus Torvalds
2024-05-01 21:49 ` Paul E. McKenney
2024-05-01 22:32 ` Paul E. McKenney
2024-05-02 16:37 ` Boqun Feng
2024-05-03 23:59 ` Paul E. McKenney
2024-05-04 0:14 ` Linus Torvalds
2024-05-04 5:08 ` Paul E. McKenney
2024-05-04 17:50 ` Linus Torvalds
2024-05-04 18:18 ` Paul E. McKenney
2024-05-04 19:11 ` Linus Torvalds
2024-05-04 19:25 ` Linus Torvalds
2024-05-04 22:17 ` Paul E. McKenney
2024-05-04 22:04 ` Paul E. McKenney
2024-05-02 14:14 ` Marco Elver
2024-05-02 16:42 ` Tetsuo Handa
2024-05-02 17:20 ` Marco Elver
2024-05-02 17:29 ` Linus Torvalds
2024-05-02 18:14 ` Al Viro
2024-05-02 19:29 ` Marco Elver [this message]
2024-05-02 23:54 ` Tetsuo Handa
2024-05-03 1:12 ` Linus Torvalds
2023-04-23 13:28 ` [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4) Tetsuo Handa
2023-04-23 14:00 ` Greg Kroah-Hartman
2023-04-23 14:03 ` Greg Kroah-Hartman
2023-04-23 14:17 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANpmjNN0n9UkYheZyBCQykrLYM9EDsmkp41a=x4hbgYyKDPZxw@mail.gmail.com' \
--to=elver@google.com \
--cc=arnd@kernel.org \
--cc=dvyukov@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nathan@kernel.org \
--cc=paulmck@kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).