Re: [PATCH] relay: handle alloc_percpu returning NULL in relay_open

[Andrew Donnellan <ajd@linux.ibm.com>]

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@linux.ibm.com>


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