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

[Andrew Donnellan]

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@xxxxxxxxxxxxx>


--
Andrew Donnellan              OzLabs, ADL Canberra
ajd@xxxxxxxxxxxxx             IBM Australia Limited