linux-kernel - Re: [PATCH] mm: introduce sanity check on dirty ratio sysctl value

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:   Mon, 18 Sep 2017 12:22:44 +0200
From:   Jan Kara <jack@...e.cz>
To:     Yafang Shao <laoar.shao@...il.com>
Cc:     akpm@...ux-foundation.org, jack@...e.cz, hannes@...xchg.org,
        mhocko@...e.com, vdavydov.dev@...il.com, jlayton@...hat.com,
        nborisov@...e.com, tytso@....edu, mawilcox@...rosoft.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: introduce sanity check on dirty ratio sysctl value

On Mon 18-09-17 01:39:28, Yafang Shao wrote:
> we can find the logic in domain_dirty_limits() that
> when dirty bg_thresh is bigger than dirty thresh,
> bg_thresh will be set as thresh * 1 / 2.
> 	if (bg_thresh >= thresh)
> 		bg_thresh = thresh / 2;
> 
> But actually we can set dirty_background_raio bigger than
> dirty_ratio successfully. This behavior may mislead us.
> So we should do this sanity check at the beginning.
> 
> Signed-off-by: Yafang Shao <laoar.shao@...il.com>

...

>  {
> +	int old_ratio = dirty_background_ratio;
> +	unsigned long bytes;
>  	int ret;
>  
>  	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> -	if (ret == 0 && write)
> -		dirty_background_bytes = 0;
> +
> +	if (ret == 0 && write) {
> +		if (vm_dirty_ratio > 0) {
> +			if (dirty_background_ratio >= vm_dirty_ratio)
> +				ret = -EINVAL;
> +		} else if (vm_dirty_bytes > 0) {
> +			bytes = global_dirtyable_memory() * PAGE_SIZE *
> +					dirty_background_ratio / 100;
> +			if (bytes >= vm_dirty_bytes)
> +				ret = -EINVAL;
> +		}
> +
> +		if (ret == 0)
> +			dirty_background_bytes = 0;
> +		else
> +			dirty_background_ratio = old_ratio;
> +	}
> +

How about implementing something like

bool vm_dirty_settings_valid(void)

helper which would validate whether current dirtiness settings are
consistent. That way we would not have to repeat very similar checks four
times. Also the arithmetics in:

global_dirtyable_memory() * PAGE_SIZE * dirty_background_ratio / 100 

could overflow so I'd prefer to first divide by 100 and then multiply by
dirty_background_ratio...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR