Re: [PATCH 5/6] listns: don't wrap around with a U64_MAX cursor

[Jan Kara]

On Wed 21-10-26 13:46:58, Christian Brauner wrote:
> do_listns() and do_listns_userns() take the last namespace id the caller
> saw and look up the first namespace after it by searching the rbtree for
> last_ns_id + 1. With U64_MAX that wraps around to zero and the search
> returns the namespace with the smallest id. So a cursor at the largest
> possible id restarts the listing from the beginning instead of failing
> with ENOENT like a cursor of U64_MAX - 1 does. That's the case for the
> unified tree, the type-specific trees and the owner tree alike.
> 
> No namespace will ever have that id. They come from a counter and a
> caller that only feeds back ids it got from us can't hit this. It's
> still wrong.
> 
> Instead of adding one to the cursor and searching for the smallest id
> that's greater or equal make the lookup helpers search for the smallest
> id that's strictly greater than the cursor. That's what both callers
> want anyway and there's nothing left to overflow.
> 
> Reproducer, unprivileged:
> 
>     #define _GNU_SOURCE
>     #include <errno.h>
>     #include <stdint.h>
>     #include <stdio.h>
>     #include <sys/syscall.h>
>     #include <unistd.h>
>     #ifndef __NR_listns
>     #define __NR_listns 470
>     #endif
>     #define CLONE_NEWUSER		0x10000000U
>     #define LISTNS_CURRENT_USER	0xffffffffffffffffULL
>     struct ns_id_req {
>     	uint32_t size, spare;
>     	uint64_t ns_id;
>     	uint32_t ns_type, spare2;
>     	uint64_t user_ns_id;
>     };
>     static void list(const char *tree, uint32_t type, uint64_t owner,
>     		 uint64_t cursor)
>     {
>     	struct ns_id_req req = {
>     		.size = sizeof(req), .ns_id = cursor, .ns_type = type,
>     		.user_ns_id = owner,
>     	};
>     	uint64_t ids[8];
>     	long ret;
>     	errno = 0;
>     	ret = syscall(__NR_listns, &req, ids, 8, 0);
>     	printf("%s cursor %llu: ret %ld errno %d", tree,
>     	       (unsigned long long)cursor, ret, errno);
>     	for (long i = 0; i < ret && i < 3; i++)
>     		printf("%s%llu", i ? "," : " ids ", (unsigned long long)ids[i]);
>     	printf("%s\n", ret > 3 ? ",..." : "");
>     }
>     int main(void)
>     {
>     	static const uint64_t cursors[] = { 0, UINT64_MAX - 1, UINT64_MAX };
>     	for (int i = 0; i < 3; i++)
>     		list("unified", 0, 0, cursors[i]);
>     	for (int i = 0; i < 3; i++)
>     		list("userns", CLONE_NEWUSER, 0, cursors[i]);
>     	for (int i = 0; i < 3; i++)
>     		list("owner", 0, LISTNS_CURRENT_USER, cursors[i]);
>     	return 0;
>     }
> 
> Before, on 7.3.0-rc7-00252-g4e3259b08c54 (vfs.fixes) in a VM:
> 
>     unified cursor 0: ret 8 errno 0 ids 1,2,3,...
>     unified cursor 18446744073709551614: ret -1 errno 2
>     unified cursor 18446744073709551615: ret 8 errno 0 ids 1,2,3,...
>     userns cursor 0: ret 1 errno 0 ids 3
>     userns cursor 18446744073709551614: ret -1 errno 2
>     userns cursor 18446744073709551615: ret 1 errno 0 ids 3
>     owner cursor 0: ret 8 errno 0 ids 1,2,4,...
>     owner cursor 18446744073709551614: ret -1 errno 2
>     owner cursor 18446744073709551615: ret 8 errno 0 ids 1,2,4,...
> 
> After:
> 
>     unified cursor 0: ret 8 errno 0 ids 1,2,3,...
>     unified cursor 18446744073709551614: ret -1 errno 2
>     unified cursor 18446744073709551615: ret -1 errno 2
>     userns cursor 0: ret 1 errno 0 ids 3
>     userns cursor 18446744073709551614: ret -1 errno 2
>     userns cursor 18446744073709551615: ret -1 errno 2
>     owner cursor 0: ret 8 errno 0 ids 1,2,4,...
>     owner cursor 18446744073709551614: ret -1 errno 2
>     owner cursor 18446744073709551615: ret -1 errno 2
> 
> Reported-by: shaos <shawnq1@xxxxxxx>
> Link: https://lore.kernel.org/r/20261020110732.959552-1-shawnq1@xxxxxxx
> Fixes: 76b6f5dfb3fd ("nstree: add listns()")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  kernel/nstree.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/nstree.c b/kernel/nstree.c
> index 831f279d174a..58b2b55fa8d2 100644
> --- a/kernel/nstree.c
> +++ b/kernel/nstree.c
> @@ -459,10 +459,10 @@ static inline int prepare_klistns(struct klistns *kls, struct ns_id_req *kreq,
>  }
>  
>  /*
> - * Lookup a namespace owned by owner with id >= ns_id.
> - * Returns the namespace with the smallest id that is >= ns_id.
> + * Lookup a namespace owned by owner with id > ns_id.
> + * Returns the namespace with the smallest id that is > ns_id.
>   */
> -static struct ns_common *lookup_ns_owner_at(u64 ns_id, struct ns_common *owner)
> +static struct ns_common *lookup_ns_owner_after(u64 ns_id, struct ns_common *owner)
>  {
>  	struct ns_common *ret = NULL;
>  	struct rb_node *node;
> @@ -476,10 +476,8 @@ static struct ns_common *lookup_ns_owner_at(u64 ns_id, struct ns_common *owner)
>  		struct ns_common *ns;
>  
>  		ns = node_to_ns_owner(node);
> -		if (ns_id <= ns->ns_id) {
> +		if (ns_id < ns->ns_id) {
>  			ret = ns;
> -			if (ns_id == ns->ns_id)
> -				break;
>  			node = node->rb_left;
>  		} else {
>  			node = node->rb_right;
> @@ -565,7 +563,7 @@ static ssize_t do_listns_userns(struct klistns *kls)
>  	 * then use it's list entry to iterate from there.
>  	 */
>  	if (kls->last_ns_id) {
> -		kls->first_ns = lookup_ns_owner_at(kls->last_ns_id + 1, ns);
> +		kls->first_ns = lookup_ns_owner_after(kls->last_ns_id, ns);
>  		if (!kls->first_ns)
>  			return -ENOENT;
>  		first_ns = kls->first_ns;
> @@ -613,10 +611,10 @@ static ssize_t do_listns_userns(struct klistns *kls)
>  }
>  
>  /*
> - * Lookup a namespace with id >= ns_id in either the unified tree or a type-specific tree.
> - * Returns the namespace with the smallest id that is >= ns_id.
> + * Lookup a namespace with id > ns_id in either the unified tree or a type-specific tree.
> + * Returns the namespace with the smallest id that is > ns_id.
>   */
> -static struct ns_common *lookup_ns_id_at(u64 ns_id, int ns_type)
> +static struct ns_common *lookup_ns_id_after(u64 ns_id, int ns_type)
>  {
>  	struct ns_common *ret = NULL;
>  	struct ns_tree_root *ns_tree = NULL;
> @@ -643,13 +641,8 @@ static struct ns_common *lookup_ns_id_at(u64 ns_id, int ns_type)
>  		else
>  			ns = node_to_ns_unified(node);
>  
> -		if (ns_id <= ns->ns_id) {
> -			if (ns_type)
> -				ret = node_to_ns(node);
> -			else
> -				ret = node_to_ns_unified(node);
> -			if (ns_id == ns->ns_id)
> -				break;
> +		if (ns_id < ns->ns_id) {
> +			ret = ns;
>  			node = node->rb_left;
>  		} else {
>  			node = node->rb_right;
> @@ -708,7 +701,7 @@ static ssize_t do_listns(struct klistns *kls)
>  	}
>  
>  	if (kls->last_ns_id) {
> -		kls->first_ns = lookup_ns_id_at(kls->last_ns_id + 1, ns_type);
> +		kls->first_ns = lookup_ns_id_after(kls->last_ns_id, ns_type);
>  		if (!kls->first_ns)
>  			return -ENOENT;
>  		first_ns = kls->first_ns;
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR