There is a dangling reference to pipe in a watch_queue after clearing it.
Thus, NULL that pointer while clearing.
This change renders wqueue->defunct superfluous, as the latter is only used
to check if watch_queue is cleared. With this change, the pipe is NULLed
while clearing, so we can just check if the pipe is NULL.
Extending comment for watch_queue->pipe in the definition of watch_queue
made the comment conventionally too long (it was already past 80 chars),
so I have changed the struct annotations to be kerneldoc-styled, so that
I can extend the comment mentioning that the pipe is NULL when watch_queue
is cleared. In the process, I have also hopefully improved documentation
by documenting things which weren't documented before.
Changes in v2:
- Merged the NULLing and removing defunct patches.
- Removed READ_ONCE barrier in lock_wqueue().
- Improved and fixed errors in struct docs.
- Better commit messages.
Siddh Raman Pant (2):
include/linux/watch_queue: Improve documentation
kernel/watch_queue: NULL the dangling *pipe, and use it for clear
check
include/linux/watch_queue.h | 100 ++++++++++++++++++++++++++----------
kernel/watch_queue.c | 12 ++---
2 files changed, 79 insertions(+), 33 deletions(-)
--
2.35.1
NULL the dangling pipe reference while clearing watch_queue.
If not done, a reference to a freed pipe remains in the watch_queue,
as this function is called before freeing a pipe in free_pipe_info()
(see line 834 of fs/pipe.c).
The sole use of wqueue->defunct is for checking if the watch queue has
been cleared, but wqueue->pipe is also NULLed while clearing.
Thus, wqueue->defunct is superfluous, as wqueue->pipe can be checked
for NULL. Hence, the former can be removed.
Signed-off-by: Siddh Raman Pant <[email protected]>
---
include/linux/watch_queue.h | 4 +---
kernel/watch_queue.c | 12 ++++++------
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
index 7f8b1f15634b..d72ad82a4435 100644
--- a/include/linux/watch_queue.h
+++ b/include/linux/watch_queue.h
@@ -55,7 +55,7 @@ struct watch_filter {
*
* @rcu: RCU head
* @filter: Filter to use on watches
- * @pipe: The pipe we're using as a buffer
+ * @pipe: The pipe we're using as a buffer, NULL when queue is cleared/closed
* @watches: Contributory watches
* @notes: Preallocated notifications
* @notes_bitmap: Allocation bitmap for notes
@@ -63,7 +63,6 @@ struct watch_filter {
* @lock: To serialize accesses and removes
* @nr_notes: Number of notes
* @nr_pages: Number of pages in notes[]
- * @defunct: True when queues closed
*/
struct watch_queue {
struct rcu_head rcu;
@@ -76,7 +75,6 @@ struct watch_queue {
spinlock_t lock;
unsigned int nr_notes;
unsigned int nr_pages;
- bool defunct;
};
/**
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index a6f9bdd956c3..a70ddfd622ee 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -43,7 +43,7 @@ MODULE_LICENSE("GPL");
static inline bool lock_wqueue(struct watch_queue *wqueue)
{
spin_lock_bh(&wqueue->lock);
- if (unlikely(wqueue->defunct)) {
+ if (unlikely(!wqueue->pipe)) {
spin_unlock_bh(&wqueue->lock);
return false;
}
@@ -105,9 +105,6 @@ static bool post_one_notification(struct watch_queue *wqueue,
unsigned int head, tail, mask, note, offset, len;
bool done = false;
- if (!pipe)
- return false;
-
spin_lock_irq(&pipe->rd_wait.lock);
mask = pipe->ring_size - 1;
@@ -603,8 +600,11 @@ void watch_queue_clear(struct watch_queue *wqueue)
rcu_read_lock();
spin_lock_bh(&wqueue->lock);
- /* Prevent new notifications from being stored. */
- wqueue->defunct = true;
+ /*
+ * This pipe will get freed by the caller free_pipe_info().
+ * Removing this reference also prevents new notifications.
+ */
+ wqueue->pipe = NULL;
while (!hlist_empty(&wqueue->watches)) {
watch = hlist_entry(wqueue->watches.first, struct watch, queue_node);
--
2.35.1
Introduce kerneldoc-style comments, and document a couple of things
explicitly.
Signed-off-by: Siddh Raman Pant <[email protected]>
---
include/linux/watch_queue.h | 102 ++++++++++++++++++++++++++----------
1 file changed, 75 insertions(+), 27 deletions(-)
diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
index fc6bba20273b..7f8b1f15634b 100644
--- a/include/linux/watch_queue.h
+++ b/include/linux/watch_queue.h
@@ -18,57 +18,103 @@
struct cred;
+/**
+ * struct watch_type_filter - Filter on watch type
+ *
+ * @type: Type of watch_notification
+ * @subtype_filter: Bitmask of subtypes to filter on
+ * @info_filter: Filter on watch_notification::info
+ * @info_mask: Mask of relevant bits in info_filter
+ */
struct watch_type_filter {
enum watch_notification_type type;
- __u32 subtype_filter[1]; /* Bitmask of subtypes to filter on */
- __u32 info_filter; /* Filter on watch_notification::info */
- __u32 info_mask; /* Mask of relevant bits in info_filter */
+ __u32 subtype_filter[1];
+ __u32 info_filter;
+ __u32 info_mask;
};
+/**
+ * struct watch_filter - Filter on watch
+ *
+ * @rcu: RCU head (in union with type_filter)
+ * @type_filter: Bitmask of accepted types (in union with rcu)
+ * @nr_filters: Number of filters
+ * @filters: Array of watch_type_filter
+ */
struct watch_filter {
union {
struct rcu_head rcu;
- /* Bitmask of accepted types */
DECLARE_BITMAP(type_filter, WATCH_TYPE__NR);
};
- u32 nr_filters; /* Number of filters */
+ u32 nr_filters;
struct watch_type_filter filters[];
};
+/**
+ * struct watch_queue - General notification queue
+ *
+ * @rcu: RCU head
+ * @filter: Filter to use on watches
+ * @pipe: The pipe we're using as a buffer
+ * @watches: Contributory watches
+ * @notes: Preallocated notifications
+ * @notes_bitmap: Allocation bitmap for notes
+ * @usage: Object usage count
+ * @lock: To serialize accesses and removes
+ * @nr_notes: Number of notes
+ * @nr_pages: Number of pages in notes[]
+ * @defunct: True when queues closed
+ */
struct watch_queue {
struct rcu_head rcu;
struct watch_filter __rcu *filter;
- struct pipe_inode_info *pipe; /* The pipe we're using as a buffer */
- struct hlist_head watches; /* Contributory watches */
- struct page **notes; /* Preallocated notifications */
- unsigned long *notes_bitmap; /* Allocation bitmap for notes */
- struct kref usage; /* Object usage count */
+ struct pipe_inode_info *pipe;
+ struct hlist_head watches;
+ struct page **notes;
+ unsigned long *notes_bitmap;
+ struct kref usage;
spinlock_t lock;
- unsigned int nr_notes; /* Number of notes */
- unsigned int nr_pages; /* Number of pages in notes[] */
- bool defunct; /* T when queues closed */
+ unsigned int nr_notes;
+ unsigned int nr_pages;
+ bool defunct;
};
-/*
- * Representation of a watch on an object.
+/**
+ * struct watch - Representation of a watch on an object
+ *
+ * @rcu: RCU head (in union with info_id)
+ * @info_id: ID to be OR'd in to info field (in union with rcu)
+ * @queue: Queue to post events to
+ * @queue_node: Link in queue->watches
+ * @watch_list: Link in watch_list->watchers
+ * @list_node: The list node
+ * @cred: Creds of the owner of the watch
+ * @private: Private data for the watched object
+ * @id: Internal identifier
+ * @usage: Object usage count
*/
struct watch {
union {
struct rcu_head rcu;
- u32 info_id; /* ID to be OR'd in to info field */
+ u32 info_id;
};
- struct watch_queue __rcu *queue; /* Queue to post events to */
- struct hlist_node queue_node; /* Link in queue->watches */
+ struct watch_queue __rcu *queue;
+ struct hlist_node queue_node;
struct watch_list __rcu *watch_list;
- struct hlist_node list_node; /* Link in watch_list->watchers */
- const struct cred *cred; /* Creds of the owner of the watch */
- void *private; /* Private data for the watched object */
- u64 id; /* Internal identifier */
- struct kref usage; /* Object usage count */
+ struct hlist_node list_node;
+ const struct cred *cred;
+ void *private;
+ u64 id;
+ struct kref usage;
};
-/*
- * List of watches on an object.
+/**
+ * struct watch_list - List of watches on an object
+ *
+ * @rcu: RCU head
+ * @watchers: List head
+ * @release_watch: Function to release watch
+ * @lock: To protect addition and removal of watches
*/
struct watch_list {
struct rcu_head rcu;
@@ -118,8 +164,10 @@ static inline void remove_watch_list(struct watch_list *wlist, u64 id)
}
/**
- * watch_sizeof - Calculate the information part of the size of a watch record,
- * given the structure size.
+ * watch_sizeof() - Calculate the information part of the size of a watch
+ * record, given the structure size.
+ *
+ * @STRUCT: The structure whose size is to be given
*/
#define watch_sizeof(STRUCT) (sizeof(STRUCT) << WATCH_INFO_LENGTH__SHIFT)
--
2.35.1
On Sat, 06 Aug 2022 13:13:40 +0530 Siddh Raman Pant wrote:
> There is a dangling reference to pipe in a watch_queue after clearing it.
> Thus, NULL that pointer while clearing.
>
> This change renders wqueue->defunct superfluous, as the latter is only used
> to check if watch_queue is cleared. With this change, the pipe is NULLed
> while clearing, so we can just check if the pipe is NULL.
>
> Extending comment for watch_queue->pipe in the definition of watch_queue
> made the comment conventionally too long (it was already past 80 chars),
> so I have changed the struct annotations to be kerneldoc-styled, so that
> I can extend the comment mentioning that the pipe is NULL when watch_queue
> is cleared. In the process, I have also hopefully improved documentation
> by documenting things which weren't documented before.
>
> Changes in v2:
> - Merged the NULLing and removing defunct patches.
> - Removed READ_ONCE barrier in lock_wqueue().
> - Improved and fixed errors in struct docs.
> - Better commit messages.
>
> Siddh Raman Pant (2):
> include/linux/watch_queue: Improve documentation
> kernel/watch_queue: NULL the dangling *pipe, and use it for clear
> check
>
> include/linux/watch_queue.h | 100 ++++++++++++++++++++++++++----------
> kernel/watch_queue.c | 12 ++---
> 2 files changed, 79 insertions(+), 33 deletions(-)
>
> --
> 2.35.1
Hello,
Maybe the above quoted patch was missed due to the
then-ongoing merge window. This can be found on:
https://lore.kernel.org/lkml/[email protected]/
This patch was posted 2 weeks ago, with changes as requested.
With the merge window closed, may I request for another look at
this patch?
Thanks,
Siddh