Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] relay: handle alloc_percpu returning NULL in relay_open
To:     Michael Ellerman <mpe@ellerman.id.au>,
        Daniel Axtens <dja@axtens.net>, linux-kernel@vger.kernel.org,
        viro@zeniv.linux.org.uk
Cc:     akash.goel@intel.com,
        syzbot+1e925b4b836afe85a1c6@syzkaller-ppc64.appspotmail.com,
        syzbot+587b2421926808309d21@syzkaller-ppc64.appspotmail.com,
        syzbot+58320b7171734bf79d26@syzkaller.appspotmail.com,
        syzbot+d6074fb08bdb2e010520@syzkaller.appspotmail.com
References: <20191129013745.7168-1-dja@axtens.net>
 <87d0dbffbk.fsf@mpe.ellerman.id.au>
From:   Andrew Donnellan <ajd@linux.ibm.com>
Date:   Fri, 29 Nov 2019 23:42:33 +1100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <87d0dbffbk.fsf@mpe.ellerman.id.au>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <4415adee-5c12-3d65-d44a-34ee4eb00fed@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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