Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 18 Feb 2021 11:45:10 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Chris Down <chris@chrisdown.name>
Cc:     linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        John Ogness <john.ogness@linutronix.de>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Kees Cook <keescook@chromium.org>, kernel-team@fb.com
Subject: Re: code style: Re: [PATCH v4] printk: Userspace format enumeration
 support
Message-ID: <YC5FNkSkQ+ez1JvT@alley>
References: <YCafCKg2bAlOw08H@chrisdown.name>
 <YCv9Xb7ePnDy9xRf@alley>
 <YCwAbGoVuZJspcx5@chrisdown.name>
 <YC0/xQTephcfo6GL@alley>
 <YC1DjeZmBWmJe35c@chrisdown.name>
 <YC1FFVCNQpaJwO/N@chrisdown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YC1FFVCNQpaJwO/N@chrisdown.name>
Precedence: bulk

On Wed 2021-02-17 16:32:21, Chris Down wrote:
> Chris Down writes:
> > open(f);
> >  debugfs_file_get(f);
> >  fops->open();
> >    inode->private = ps;
> >  debugfs_file_put(f);
> > 
> > remove_printk_fmt_sec(); /* kfree ps */
> > 
> > read(f);
> >  debugfs_file_get(f);
> >  fops->read();
> >    ps = inode->private;  /* invalid */
> >  debugfs_file_put(f);
> 
> Er, sorry, inode->private is populated at creation time, not at open(). The
> same general concern applies though -- as far as I can tell there's some
> period where we may be able to _read() and `ps` has already been freed.

Honestly, I am a bit lost here. Also I have realized that you needed to
release the struct from the module going notifier. And there you have
only pointer to struct module.

The thing is that I do not see similar tricks anywhere else. Well, other
users are easier because they create the debugfs file for it own purpose.
In our case, we actually create the file for another module.

Anyway, we are going to use the seq_buf iterator API. IMHO, the
seq_buf iterator functions should be able to get the structure
directly via the data pointer.

I wonder if it is similar to proc_seq_open() and proc_seq_release().
It is using the seq_buf iterator as well. It is created used
by proc_create_seq_private(). This API is used, for example,
in decnet_init(). It is a module, so there must be the similar
problems. All data are gone when the module is removed.

The only remaining problem is the module going notifier. For this
purpose, we could store the pointer to struct module. There
are many other fields for similar purposes. I am pretty sure that
the module loader maintainer will agree.

The result will be using some existing patterns. It should reduce
problems with possible races. It should make the life easier for
all involved APIs.

Best Regards,
Petr