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 PHC
Open Source and information security mailing list archives

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

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

Date:   Mon, 18 Sep 2017 19:36:56 +0800
From:   Yafang Shao <laoar.shao@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     akpm@...ux-foundation.org, Johannes Weiner <hannes@...xchg.org>,
        mhocko@...e.com, vdavydov.dev@...il.com, jlayton@...hat.com,
        nborisov@...e.com, "Theodore Ts'o" <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

2017-09-18 18:22 GMT+08:00 Jan Kara <jack@...e.cz>:
> 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.

That seems a smarter way.

> 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...
>
Oh, yes. It could overflow.

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


I will reimplement it and submit a new patch.

Thanks
Yafang