linux-kernel - Re: [PATCH] fs: Improve eventpoll logging to stop indicting timerfd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Mar 2021 14:04:01 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Manish Varma <varmam@...gle.com>,
        Alexander Viro <viro@...iv.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Manish Varma <varmam@...gle.com>,
        Kelly Rossmoyer <krossmo@...gle.com>
Subject: Re: [PATCH] fs: Improve eventpoll logging to stop indicting timerfd

Manish,

On Mon, Mar 01 2021 at 19:49, Manish Varma wrote:

> All together, that will give us names like the following:
>
> 1) timerfd file descriptor: [timerfd14:system_server]
> 2) eventpoll top-level per-process wakesource: epoll:system_server
> 3) eventpoll-on-timerfd per-descriptor wakesource:
> epollitem:system_server.[timerfd14:system_server]

All together that should be splitted up into a change to eventpoll and
timerfd.

> diff --git a/fs/timerfd.c b/fs/timerfd.c
> index c5509d2448e3..4249e8c9a38c 100644
> --- a/fs/timerfd.c
> +++ b/fs/timerfd.c
> @@ -46,6 +46,8 @@ struct timerfd_ctx {
>  	bool might_cancel;
>  };
>  
> +static atomic_t instance_count = ATOMIC_INIT(0);

instance_count is misleading as it does not do any accounting of
instances as the name suggests.

>  static LIST_HEAD(cancel_list);
>  static DEFINE_SPINLOCK(cancel_lock);
>  
> @@ -391,6 +393,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
>  {
>  	int ufd;
>  	struct timerfd_ctx *ctx;
> +	char task_comm_buf[sizeof(current->comm)];
> +	char file_name_buf[32];
> +	int instance;
>  
>  	/* Check the TFD_* constants for consistency.  */
>  	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
> @@ -427,7 +432,11 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
>  
>  	ctx->moffs = ktime_mono_to_real(0);
>  
> -	ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
> +	instance = atomic_inc_return(&instance_count);
> +	get_task_comm(task_comm_buf, current);

How is current->comm supposed to be unique? And with a wrapping counter
like the above you can end up with identical file descriptor names.

What's wrong with simply using the PID which is guaranteed to be unique
for the life time of a process/task?

> +	snprintf(file_name_buf, sizeof(file_name_buf), "[timerfd%d:%s]",
> +		 instance, task_comm_buf);
> +	ufd = anon_inode_getfd(file_name_buf, &timerfd_fops, ctx,
>  			       O_RDWR | (flags & TFD_SHARED_FCNTL_FLAGS));
>  	if (ufd < 0)
>  		kfree(ctx);

I actually wonder, whether this should be part of anon_inode_get*().

Aside of that this is a user space visible change both for eventpoll and
timerfd.

Have you carefully investigated whether there is existing user space
which might depend on the existing naming conventions?

The changelog is silent about this...

Thanks,

        tglx