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;
Date:   Wed, 1 Aug 2018 16:38:40 +0200
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Greg Kurz <groug@kaod.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: <20180801143840.GA21463@nautica>
References: <20180730093101.GA7894@nautica>
 <1532943263-24378-1-git-send-email-asmadeus@codewreck.org>
 <20180801161413.0523a821@bahia.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20180801161413.0523a821@bahia.lan>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;
> >  
> > -	req->tc = p9_fcall_alloc(alloc_msize);
> > -	req->rc = p9_fcall_alloc(alloc_msize);
> > -	if (!req->tc || !req->rc)
> > +	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?

(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?)

-- 
Dominique