linux-kernel - Re: [PATCH] relay: handle alloc_percpu returning NULL in relay

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, 29 Nov 2019 23:42:33 +1100
From:   Andrew Donnellan <ajd@...ux.ibm.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Daniel Axtens <dja@...ens.net>, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk
Cc:     akash.goel@...el.com,
        syzbot+1e925b4b836afe85a1c6@...kaller-ppc64.appspotmail.com,
        syzbot+587b2421926808309d21@...kaller-ppc64.appspotmail.com,
        syzbot+58320b7171734bf79d26@...kaller.appspotmail.com,
        syzbot+d6074fb08bdb2e010520@...kaller.appspotmail.com
Subject: Re: [PATCH] relay: handle alloc_percpu returning NULL in relay_open

On 29/11/19 3:59 pm, Michael Ellerman wrote:
>> diff --git a/kernel/relay.c b/kernel/relay.c
>> index ade14fb7ce2e..a376cc6b54ec 100644
>> --- a/kernel/relay.c
>> +++ b/kernel/relay.c
>> @@ -580,7 +580,13 @@ struct rchan *relay_open(const char *base_filename,
>>   	if (!chan)
>>   		return NULL;
>>   
>> -	chan->buf = alloc_percpu(struct rchan_buf *);
>> +	chan->buf = alloc_percpu_gfp(struct rchan_buf *,
>> +				     GFP_KERNEL | __GFP_NOWARN);
>> +	if (!chan->buf) {
>> +		kfree(chan);
>> +		return NULL;
>> +	}
>> +
>>   	chan->version = RELAYFS_CHANNEL_VERSION;
>>   	chan->n_subbufs = n_subbufs;
>>   	chan->subbuf_size = subbuf_size;
> 
> This looks right to me. The kfree + direct return is correct, there's
> nothing else that needs tear down in this function.
> 
> I think I'm 50/50 on the __GFP_NOWARN. We're only asking for 8 bytes per
> cpu, and if that fails the system is pretty sick, so a warning could be
> helpful. There's also logic in the percpu allocator to limit the number
> of warnings printed. But see what others think.

mpe was wondering why we didn't see a message printed from the percpu 
allocator - the answer appears to be that we hit this case when the 
process is killed while the percpu allocator is waiting for 
pcpu_alloc_mutex, in which case it bails out without printing a warning.

It looks to me like that case doesn't warrant a warning message, while a 
failing allocation for other reasons should probably get a warning.

But whatever, otherwise this patch looks good to me. I've told our 
powerpc syzbot to test it.

Reviewed-by: Andrew Donnellan <ajd@...ux.ibm.com>


-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@...ux.ibm.com             IBM Australia Limited