linux-kernel - Re: [patch V2 3/5] posix-cpu-timers: Provide mechanisms to defer timer handling to task

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]

Message-ID: <20200717172627.GC6067@redhat.com>
Date:   Fri, 17 Jul 2020 19:26:28 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        John Stultz <john.stultz@...aro.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [patch V2 3/5] posix-cpu-timers: Provide mechanisms to defer
 timer handling to task_work

Looks correct to me, but I forgot everything about posix-timers.c

this obviously means that the expired timer won't fire until the
task returns to user-mode but probably we don't care.

One cosmetic nit below,

On 07/16, Thomas Gleixner wrote:
>
> +#ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
> +void posix_cpu_timers_work(struct callback_head *work);
> +
> +static inline void posix_cputimer_init_work(struct posix_cputimers *pct)
> +{
> +	pct->task_work.func = posix_cpu_timers_work;

init_task_work() ?

> +}
> +#else
> +static inline void posix_cputimer_init_work(struct posix_cputimers *pct) { }
> +#endif
> +
>  static inline void posix_cputimers_init(struct posix_cputimers *pct)
>  {
>  	memset(pct, 0, sizeof(*pct));
>  	pct->bases[0].nextevt = U64_MAX;
>  	pct->bases[1].nextevt = U64_MAX;
>  	pct->bases[2].nextevt = U64_MAX;
> +	posix_cputimer_init_work(pct);
>  }

And I can't resist. I know this is a common practice, please ignore, but to me

	static inline void posix_cputimers_init(struct posix_cputimers *pct)
	{
		memset(pct, 0, sizeof(*pct));
		pct->bases[0].nextevt = U64_MAX;
		pct->bases[1].nextevt = U64_MAX;
		pct->bases[2].nextevt = U64_MAX;
	#ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
		init_task_work(&pct->task_work, posix_cpu_timers_work);
	#endif
	}

looks better than 2 posix_cputimer_init_work() definitions above.

Note also that signal_struct->posix_cputimers.task_work is never used, perhaps
it would be better to move this task_work into task_struct? This way we do not
even need to change posix_cputimers_init(), we call simply initialize
init_task.posix_task_work.

Oleg.