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]

Date:   Fri, 17 Jul 2020 20:35:46 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Oleg Nesterov <oleg@...hat.com>
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

Oleg,

Oleg Nesterov <oleg@...hat.com> writes:
> Looks correct to me, but I forgot everything about posix-timers.c

that's not a problem because this is about posix-cpu-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.

If the signal goes to the task itself it does not matter at all because
it's going to be delivered when the task goes out to user space.

If the signal goes to a supervisor process, then it will be slightly
delayed but I could not find a problem with that at all.

I'll add more reasoning to the changelog on V3.

>> +#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() ?

Yeah.

>> +}
>> +#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.

Gah, I hate ifdefs in the middle of the code :)

> 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.

Let me look into that.

Thanks,

        tglx