On Wed, 1 Aug 2018 16:38:40 +0200 Dominique Martinet <asmad...@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.
