From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from 4.mo173.mail-out.ovh.net ([46.105.34.219]:60546 "EHLO
        4.mo173.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S2389627AbeHARHd (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 1 Aug 2018 13:07:33 -0400
Received: from player697.ha.ovh.net (unknown [10.109.143.246])
        by mo173.mail-out.ovh.net (Postfix) with ESMTP id EEDF1C7AD8
        for <linux-fsdevel@vger.kernel.org>; Wed,  1 Aug 2018 17:03:51 +0200 (CEST)
Date: Wed, 1 Aug 2018 17:03:44 +0200
From: Greg Kurz <groug@kaod.org>
To: Dominique Martinet <asmadeus@codewreck.org>
Cc: v9fs-developer@lists.sourceforge.net,
        linux-fsdevel@vger.kernel.org,
        Matthew Wilcox <willy@infradead.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [V9fs-developer] [PATCH 1/2] net/9p: embed fcall in req to
 round down buffer allocs
Message-ID: <20180801170344.2ce3a762@bahia.lan>
In-Reply-To: <20180801143840.GA21463@nautica>
References: <20180730093101.GA7894@nautica>
        <1532943263-24378-1-git-send-email-asmadeus@codewreck.org>
        <20180801161413.0523a821@bahia.lan>
        <20180801143840.GA21463@nautica>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Wed, 1 Aug 2018 16:38:40 +0200
Dominique Martinet <asmadeus@codewreck.org> wrote:

> Greg Kurz wrote on Wed, Aug 01, 2018:
> > > @@ -263,13 +261,13 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
> > >  	if (!req)
> > >  		return NULL;
> > >  
> > > +	if (p9_fcall_alloc(&req->tc, alloc_msize))
> > > +		goto free;
> > > +	if (p9_fcall_alloc(&req->rc, alloc_msize))
> > >  		goto free;  
> > 
> > Hmm... if the first allocation fails, we will kfree() req->rc.sdata.
> > 
> > Are we sure we won't have a stale pointer or uninitialized data in
> > there ?  
> 
> Yeah, Jun pointed that out and I have a v2 that only frees as needed
> with an extra goto (I sent an incremental diff in my reply to his
> comment here[1])
> 
> [1] https://lkml.kernel.org/r/20180731011256.GA30388@nautica
> 
> > And even if we don't with the current code base, this is fragile and
> > could be easily broken.
> > 
> > I think you should drop this hunk and rather rename p9_fcall_alloc() to
> > p9_fcall_alloc_sdata() instead, since this is what the function is
> > actually doing with this patch applied.  
> 
> Hmm. I agree the naming isn't accurate, but even if we rename it we'll
> need to pass a pointer to fcall as argument as it inits its capacity.
> p9_fcall_init(fc, msize) might be simpler?
> 

Ah yes you're right... alloc is a bit misleading then. I agree that
p9_fcall_init() is more appropriate in this case.

And maybe you should introduce p9_fcall_fini() or _release() for
completeness. It would only do kfree() for a start, but it would
then evolve to be like the p9_fcall_kfree() function from patch 2.

> (I'm not sure I follow what you mean by 'drop this hunk', to be honest,
> did you want a single function call to init both maybe?)
> 

I was meaning "keep the same logic in p9_tag_alloc()", something like:

	req->tc.sdata = p9_fcall_alloc_sdata(&req->tc, alloc_msize);
	req->rc.sdata = p9_fcall_alloc_sdata(&req->tc, alloc_msize);
	if (!req->tc.sdata || !req->rc.sdata)

But I agree the way you did is cleaner.