On 25 April 2016 at 04:47, Suzuki K Poulose <[email protected]> wrote: > On 22/04/16 18:14, Mathieu Poirier wrote: >> >> In function tmc_open(), if tmc_read_prepare() fails variable >> drvdata->read_count is not decremented, causing unwanted >> access to drvdata->buf and very likely, a crash dump. >> >> By moving the incrementation to a place where we know things >> are stable this kind of situation is avoided. >> >> Signed-off-by: Mathieu Poirier <[email protected]> >> Reviewed-by: Suzuki K Poulose <[email protected]> >> --- >> drivers/hwtracing/coresight/coresight-tmc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c >> b/drivers/hwtracing/coresight/coresight-tmc.c >> index e8e12a9b917a..55806352b1f1 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc.c >> @@ -121,13 +121,14 @@ static int tmc_open(struct inode *inode, struct file >> *file) >> struct tmc_drvdata, >> miscdev); >> int ret = 0; >> > > On a second thought, I think there could be a race here. > > >> - if (drvdata->read_count++) >> + if (drvdata->read_count) >> goto out; >> >> ret = tmc_read_prepare(drvdata); >> if (ret) >> return ret; >> out: > > > What prevents someone else doing a release() on the file when we get here, > without > incrementing the read_count ? Also, read_count accesses are not protected. > Either should > be covered by the drvdata->spinlock or convert it to atomic.
I agree - I'll move it to an atomic type. Thanks, Mathieu > > > >> + drvdata->read_count++; >> nonseekable_open(inode, file); > > > > Cheers > Suzuki
