Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> writes: > Hi, > > While looking for labels with a spurious order in the linux kernel code, > I came across code around: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/vc4/vc4_gem.c#n106 > > The error handling in this for loop looks spurious to me: > - if 'drm_gem_handle_create' fails, we leak 'bo_state' > This is freed after the 'copy_to_user' > - if 'drm_gem_handle_create' fails, then we have 'state->bo_count = > i - 1' > This looks odd. 'state->bo_count = i' sounds more logical to me. > - as 'state->bo_count' is updated, this could mean than the > 'copy_to_user' should be performed. The point of reducing the bo_count was presumably for freeing the handles we had created before returning the error, which I forgot to actually do (userspace won't know about the new handles to go and free them itself). This is not that big a deal, as this interface is for use by a root-only, one-shot app that gets the hang state and dumps it to disk, so it can be processed later to try to debug the hang. Want to put together a patch that frees the handles (and bo_state), though, for correctness? Don't worry about copying any partial data out on error -- the userspace tool just exits.
Attachment:
signature.asc
Description: PGP signature
