linux-kernel - Re: [PATCH V3] KSM: allow dedup all tasks memory

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 for Android: free password hash cracker in your pocket

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

Message-ID: <d45addefdf05b84af96fb494d52b4ec4@natalenko.name>
Date:   Tue, 13 Nov 2018 17:33:49 +0100
From:   Oleksandr Natalenko <oleksandr@...alenko.name>
To:     timofey.titovets@...esis.ru
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, nefelim4ag@...il.com, willy@...radead.org
Subject: Re: [PATCH V3] KSM: allow dedup all tasks memory

So,

> …snip…
> +static int ksm_seeker_thread(void *nothing)
> +{
> +	pid_t last_pid = 1;
> +	pid_t curr_pid;
> +	struct task_struct *task;
> +
> +	set_freezable();
> +	set_user_nice(current, 5);
> +
> +	while (!kthread_should_stop()) {
> +		wait_while_offlining();
> +
> +		try_to_freeze();
> +
> +		if (!ksm_mode_always()) {
> +			wait_event_freezable(ksm_seeker_thread_wait,
> +				ksm_mode_always() || kthread_should_stop());
> +			continue;
> +		}
> +
> +		/*
> +		 * import one task's vma per run
> +		 */
> +		read_lock(&tasklist_lock);
> +
> +		/* Try always get next task */
> +		for_each_process(task) {
> +			curr_pid = task_pid_nr(task);
> +			if (curr_pid == last_pid) {
> +				task = next_task(task);
> +				break;
> +			}
> +
> +			if (curr_pid > last_pid)
> +				break;
> +		}
> +
> +		last_pid = task_pid_nr(task);
> +		ksm_import_task_vma(task);

This seems to be a bad idea. ksm_import_task_vma() may sleep with 
tasklist_lock being held. Thus, IIUC, you'll get this:

[ 1754.410322] BUG: scheduling while atomic: ksmd_seeker/50/0x00000002
…
[ 1754.410444] Call Trace:
[ 1754.410455]  dump_stack+0x5c/0x80
[ 1754.410460]  __schedule_bug.cold.19+0x38/0x51
[ 1754.410464]  __schedule+0x11dc/0x2080
[ 1754.410483]  schedule+0x32/0xb0
[ 1754.410487]  rwsem_down_write_failed+0x15d/0x240
[ 1754.410496]  call_rwsem_down_write_failed+0x13/0x20
[ 1754.410499]  down_write+0x20/0x30
[ 1754.410502]  ksm_import_task_vma+0x22/0x70
[ 1754.410505]  ksm_seeker_thread+0x134/0x1c0
[ 1754.410512]  kthread+0x113/0x130
[ 1754.410518]  ret_from_fork+0x35/0x40

I think you may want to get a reference to task_struct before releasing 
tasklist_lock, and then put it after ksm_import_task_vma() does its job.

> +		read_unlock(&tasklist_lock);
> +
> +		schedule_timeout_interruptible(
> +			msecs_to_jiffies(ksm_thread_seeker_sleep_millisecs));
> +	}
> +	return 0;
> +}
> …snip…

-- 
   Oleksandr Natalenko (post-factum)