* [PATCH] eventfs: Hold eventfs_mutex when calling callback functions
@ 2023-10-30 15:40 Steven Rostedt
2023-10-30 15:50 ` Steven Rostedt
2023-10-30 21:41 ` Naresh Kamboju
0 siblings, 2 replies; 3+ messages in thread
From: Steven Rostedt @ 2023-10-30 15:40 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mark Rutland,
"Arnd Bergmann\" <arnd@arndb.de>, naresh.kamboju@linaro.org, Beau Belgrave <beaub@linux.microsoft.com>, "
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The callback function that is used to create inodes and dentries is not
protected by anything and the data that is passed to it could become
stale. After eventfs_remove_dir() is called by the tracing system, it is
free to remove the events that are associated to that directory.
Unfortunately, that means the callbacks must not be called after that.
CPU0 CPU1
---- ----
eventfs_root_lookup() {
eventfs_remove_dir() {
mutex_lock(&event_mutex);
ei->is_freed = set;
mutex_unlock(&event_mutex);
}
kfree(event_call);
for (...) {
entry = &ei->entries[i];
r = entry->callback() {
call = data; // call == event_call above
if (call->flags ...)
[ USE AFTER FREE BUG ]
The safest way to protect this is to wrap the callback with:
mutex_lock(&eventfs_mutex);
if (!ei->is_freed)
r = entry->callback();
else
r = -1;
mutex_unlock(&eventfs_mutex);
This will make sure that the callback will not be called after it is
freed. But now it needs to be known that the callback is called while
holding internal eventfs locks, and that it must not call back into the
eventfs / tracefs system. There's no reason it should anyway, but document
that as well.
Link: https://lore.kernel.org/all/CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE7_Q@mail.gmail.com/
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
fs/tracefs/event_inode.c | 22 ++++++++++++++++++--
include/linux/tracefs.h | 43 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 7ad7496bd597..5a3cc5394294 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
entry = &ei->entries[i];
if (strcmp(name, entry->name) == 0) {
void *cdata = data;
- r = entry->callback(name, &mode, &cdata, &fops);
+ mutex_lock(&eventfs_mutex);
+ /* If ei->is_freed, then the event itself may be too */
+ if (!ei->is_freed)
+ r = entry->callback(name, &mode, &cdata, &fops);
+ else
+ r = -1;
+ mutex_unlock(&eventfs_mutex);
if (r <= 0)
continue;
ret = simple_lookup(dir, dentry, flags);
@@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
void *cdata = data;
entry = &ei->entries[i];
name = entry->name;
- r = entry->callback(name, &mode, &cdata, &fops);
+ mutex_lock(&eventfs_mutex);
+ /* If ei->is_freed, then the event itself may be too */
+ if (!ei->is_freed)
+ r = entry->callback(name, &mode, &cdata, &fops);
+ else
+ r = -1;
+ mutex_unlock(&eventfs_mutex);
if (r <= 0)
continue;
d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);
@@ -821,6 +833,10 @@ static void free_ei(struct eventfs_inode *ei)
* data = A pointer to @data, and the callback may replace it, which will
* cause the file created to pass the new data to the open() call.
* fops = the fops to use for the created file.
+ *
+ * NB. @callback is called while holding internal locks of the eventfs
+ * system. The callback must not call any code that might also call into
+ * the tracefs or eventfs system or it will risk creating a deadlock.
*/
struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent,
const struct eventfs_entry *entries,
@@ -880,6 +896,8 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
* @data: The default data to pass to the files (an entry may override it).
*
* This function creates the top of the trace event directory.
+ *
+ * See eventfs_create_dir() for use of @entries.
*/
struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent,
const struct eventfs_entry *entries,
diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
index 13359b1a35d1..7a5fe17b6bf9 100644
--- a/include/linux/tracefs.h
+++ b/include/linux/tracefs.h
@@ -23,9 +23,52 @@ struct file_operations;
struct eventfs_file;
+/**
+ * eventfs_callback - A callback function to create dynamic files in eventfs
+ * @name: The name of the file that is to be created
+ * @mode: return the file mode for the file (RW access, etc)
+ * @data: data to pass to the created file ops
+ * @fops: the file operations of the created file
+ *
+ * The evetnfs files are dynamically created. The struct eventfs_entry array
+ * is passed to eventfs_create_dir() or eventfs_create_events_dir() that will
+ * be used to create the files within those directories. When a lookup
+ * or access to a file within the directory is made, the struct eventfs_entry
+ * array is used to find a callback() with the matching name that is being
+ * referenced (for lookups, the entire array is iterated and each callback
+ * will be called).
+ *
+ * The callback will be called with @name for the name of the file to create.
+ * The callback can return less than 1 to indicate that no file should be
+ * created.
+ *
+ * If a file is to be created, then @mode should be populated with the file
+ * mode (permissions) for which the file is created for. This would be
+ * used to set the created inode i_mode field.
+ *
+ * The @data should be set to the data passed to the other file operations
+ * (read, write, etc). Note, @data will also point to the data passed in
+ * to eventfs_create_dir() or eventfs_create_events_dir(), but the callback
+ * can replace the data if it chooses to. Otherwise, the original data
+ * will be used for the file operation functions.
+ *
+ * The @fops should be set to the file operations that will be used to create
+ * the inode.
+ *
+ * NB. This callback is called while holding internal locks of the eventfs
+ * system. The callback must not call any code that might also call into
+ * the tracefs or eventfs system or it will risk creating a deadlock.
+ */
typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
const struct file_operations **fops);
+/**
+ * struct eventfs_entry - dynamically created eventfs file call back handler
+ * @name: Then name of the dynamic file in an eventfs directory
+ * @callback: The callback to get the fops of the file when it is created
+ *
+ * See evenfs_callback() typedef for how to set up @callback.
+ */
struct eventfs_entry {
const char *name;
eventfs_callback callback;
--
2.42.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] eventfs: Hold eventfs_mutex when calling callback functions
2023-10-30 15:40 [PATCH] eventfs: Hold eventfs_mutex when calling callback functions Steven Rostedt
@ 2023-10-30 15:50 ` Steven Rostedt
2023-10-30 21:41 ` Naresh Kamboju
1 sibling, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2023-10-30 15:50 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mark Rutland, arnd, naresh.kamboju,
Beau Belgrave, Ajay Kaher, Andrew Morton
I think I figured out why claws-mail adds a backslash to quotes when
sending. It allows you to add more than one Cc on a line, and I think (I
haven't tested it yet) if you add multiple names with quotes on the
same line, it thinks that it's a single name and will backslash internal
quotes. Hmm.
Anyway, I had to remove all quoted names to reply to this email. Hopefully
claws-mail doesn't screw it up again :-p
I may also need to upgrade claws-mail, as I build my own because I have
some things enabled that the distro version does not include.
-- Steve
On Mon, 30 Oct 2023 11:40:47 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> The callback function that is used to create inodes and dentries is not
> protected by anything and the data that is passed to it could become
> stale. After eventfs_remove_dir() is called by the tracing system, it is
> free to remove the events that are associated to that directory.
> Unfortunately, that means the callbacks must not be called after that.
>
> CPU0 CPU1
> ---- ----
> eventfs_root_lookup() {
> eventfs_remove_dir() {
> mutex_lock(&event_mutex);
> ei->is_freed = set;
> mutex_unlock(&event_mutex);
> }
> kfree(event_call);
>
> for (...) {
> entry = &ei->entries[i];
> r = entry->callback() {
> call = data; // call == event_call above
> if (call->flags ...)
>
> [ USE AFTER FREE BUG ]
>
> The safest way to protect this is to wrap the callback with:
>
> mutex_lock(&eventfs_mutex);
> if (!ei->is_freed)
> r = entry->callback();
> else
> r = -1;
> mutex_unlock(&eventfs_mutex);
>
> This will make sure that the callback will not be called after it is
> freed. But now it needs to be known that the callback is called while
> holding internal eventfs locks, and that it must not call back into the
> eventfs / tracefs system. There's no reason it should anyway, but document
> that as well.
>
> Link: https://lore.kernel.org/all/CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE7_Q@mail.gmail.com/
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> fs/tracefs/event_inode.c | 22 ++++++++++++++++++--
> include/linux/tracefs.h | 43 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 7ad7496bd597..5a3cc5394294 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> entry = &ei->entries[i];
> if (strcmp(name, entry->name) == 0) {
> void *cdata = data;
> - r = entry->callback(name, &mode, &cdata, &fops);
> + mutex_lock(&eventfs_mutex);
> + /* If ei->is_freed, then the event itself may be too */
> + if (!ei->is_freed)
> + r = entry->callback(name, &mode, &cdata, &fops);
> + else
> + r = -1;
> + mutex_unlock(&eventfs_mutex);
> if (r <= 0)
> continue;
> ret = simple_lookup(dir, dentry, flags);
> @@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
> void *cdata = data;
> entry = &ei->entries[i];
> name = entry->name;
> - r = entry->callback(name, &mode, &cdata, &fops);
> + mutex_lock(&eventfs_mutex);
> + /* If ei->is_freed, then the event itself may be too */
> + if (!ei->is_freed)
> + r = entry->callback(name, &mode, &cdata, &fops);
> + else
> + r = -1;
> + mutex_unlock(&eventfs_mutex);
> if (r <= 0)
> continue;
> d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);
> @@ -821,6 +833,10 @@ static void free_ei(struct eventfs_inode *ei)
> * data = A pointer to @data, and the callback may replace it, which will
> * cause the file created to pass the new data to the open() call.
> * fops = the fops to use for the created file.
> + *
> + * NB. @callback is called while holding internal locks of the eventfs
> + * system. The callback must not call any code that might also call into
> + * the tracefs or eventfs system or it will risk creating a deadlock.
> */
> struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent,
> const struct eventfs_entry *entries,
> @@ -880,6 +896,8 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
> * @data: The default data to pass to the files (an entry may override it).
> *
> * This function creates the top of the trace event directory.
> + *
> + * See eventfs_create_dir() for use of @entries.
> */
> struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent,
> const struct eventfs_entry *entries,
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index 13359b1a35d1..7a5fe17b6bf9 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -23,9 +23,52 @@ struct file_operations;
>
> struct eventfs_file;
>
> +/**
> + * eventfs_callback - A callback function to create dynamic files in eventfs
> + * @name: The name of the file that is to be created
> + * @mode: return the file mode for the file (RW access, etc)
> + * @data: data to pass to the created file ops
> + * @fops: the file operations of the created file
> + *
> + * The evetnfs files are dynamically created. The struct eventfs_entry array
> + * is passed to eventfs_create_dir() or eventfs_create_events_dir() that will
> + * be used to create the files within those directories. When a lookup
> + * or access to a file within the directory is made, the struct eventfs_entry
> + * array is used to find a callback() with the matching name that is being
> + * referenced (for lookups, the entire array is iterated and each callback
> + * will be called).
> + *
> + * The callback will be called with @name for the name of the file to create.
> + * The callback can return less than 1 to indicate that no file should be
> + * created.
> + *
> + * If a file is to be created, then @mode should be populated with the file
> + * mode (permissions) for which the file is created for. This would be
> + * used to set the created inode i_mode field.
> + *
> + * The @data should be set to the data passed to the other file operations
> + * (read, write, etc). Note, @data will also point to the data passed in
> + * to eventfs_create_dir() or eventfs_create_events_dir(), but the callback
> + * can replace the data if it chooses to. Otherwise, the original data
> + * will be used for the file operation functions.
> + *
> + * The @fops should be set to the file operations that will be used to create
> + * the inode.
> + *
> + * NB. This callback is called while holding internal locks of the eventfs
> + * system. The callback must not call any code that might also call into
> + * the tracefs or eventfs system or it will risk creating a deadlock.
> + */
> typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
> const struct file_operations **fops);
>
> +/**
> + * struct eventfs_entry - dynamically created eventfs file call back handler
> + * @name: Then name of the dynamic file in an eventfs directory
> + * @callback: The callback to get the fops of the file when it is created
> + *
> + * See evenfs_callback() typedef for how to set up @callback.
> + */
> struct eventfs_entry {
> const char *name;
> eventfs_callback callback;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] eventfs: Hold eventfs_mutex when calling callback functions
2023-10-30 15:40 [PATCH] eventfs: Hold eventfs_mutex when calling callback functions Steven Rostedt
2023-10-30 15:50 ` Steven Rostedt
@ 2023-10-30 21:41 ` Naresh Kamboju
1 sibling, 0 replies; 3+ messages in thread
From: Naresh Kamboju @ 2023-10-30 21:41 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mark Rutland,
Arnd Bergmann <arnd@arndb.de>, naresh.kamboju@linaro.org, Beau Belgrave <beaub@linux.microsoft.com>, Ajay Kaher <akaher@vmware.com>, Andrew Morton
On Mon, 30 Oct 2023 at 21:10, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> The callback function that is used to create inodes and dentries is not
> protected by anything and the data that is passed to it could become
> stale. After eventfs_remove_dir() is called by the tracing system, it is
> free to remove the events that are associated to that directory.
> Unfortunately, that means the callbacks must not be called after that.
>
> CPU0 CPU1
> ---- ----
> eventfs_root_lookup() {
> eventfs_remove_dir() {
> mutex_lock(&event_mutex);
> ei->is_freed = set;
> mutex_unlock(&event_mutex);
> }
> kfree(event_call);
>
> for (...) {
> entry = &ei->entries[i];
> r = entry->callback() {
> call = data; // call == event_call above
> if (call->flags ...)
>
> [ USE AFTER FREE BUG ]
>
> The safest way to protect this is to wrap the callback with:
>
> mutex_lock(&eventfs_mutex);
> if (!ei->is_freed)
> r = entry->callback();
> else
> r = -1;
> mutex_unlock(&eventfs_mutex);
>
> This will make sure that the callback will not be called after it is
> freed. But now it needs to be known that the callback is called while
> holding internal eventfs locks, and that it must not call back into the
> eventfs / tracefs system. There's no reason it should anyway, but document
> that as well.
>
> Link: https://lore.kernel.org/all/CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE7_Q@mail.gmail.com/
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> ---
> fs/tracefs/event_inode.c | 22 ++++++++++++++++++--
> include/linux/tracefs.h | 43 ++++++++++++++++++++++++++++++++++++++++
--
Linaro LKFT
https://lkft.linaro.org
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-30 21:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-30 15:40 [PATCH] eventfs: Hold eventfs_mutex when calling callback functions Steven Rostedt
2023-10-30 15:50 ` Steven Rostedt
2023-10-30 21:41 ` Naresh Kamboju
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 NNTP newsgroup(s).