From: khali@linux-fr.org (Jean Delvare) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Date: Tue, 12 Jul 2011 08:48:04 +0200 [thread overview] Message-ID: <20110712084804.0235de42@endymion.delvare> (raw) In-Reply-To: <3737907.jTEM1bACya@bloomfield> On Mon, 11 Jul 2011 23:49:10 +0200, Pavel Herrmann wrote: > On Monday 11 of July 2011 14:03:13 Guenter Roeck wrote: > > On Mon, 2011-07-11 at 16:36 -0400, Pavel Herrmann wrote: > > > the structure is dynamically allocated, but the pointer used to hold it > > > is a static global var. > > > > This is true only if CONFIG_SHARPSL_PM is defined, and it assumes that > > the driver is instantiated exactly once. That is pretty badly broken > > (the commit introducing it even admits that), and should be fixed. This > > does not happen CONFIG_SHARPSL_PM is not defined. If CONFIG_SHARPSL_PM > > _is_ defined in your environment, and you do have multiple instances of > > the driver (ie if you have multiple MAX1111 chips in your system), a > > severe problem is that max1111_read_channel() does not identify the > > driver instance. That can not be fixed with a mutex. > > if you don't have CONFIG_SHARPSL_PM then there is nothing calling > max1111_read, and thus any of the discussed doesn't matter This assumption of yours is incorrect. Even with CONFIG_SHARPSL_PM disabled, the max1111 driver creates sysfs attributes which, when read, call max1111_read(). What CONFIG_SHARPSL_PM adds is the in-kernel access. > AFAIK max1111 is only used in sharpsl devices (according to kernel drivers > anyways), and only one a piece. > this patch is meant to fix a crash, not make the driver code pretty just in > case someone else decides to use it. this patch also doesn't present any more > challenges for solving the multiple devices issue and would be necessary > either way, as drvdata is not thread-safe anyways (or I am badly mistaken) You are right, drvdata is not thread-safe, and this is the most obvious reason why your patch is needed. -- Jean Delvare
WARNING: multiple messages have this Message-ID (diff)
From: Jean Delvare <khali@linux-fr.org> To: linux-arm-kernel@lists.infradead.org Subject: Re: [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Date: Tue, 12 Jul 2011 06:48:04 +0000 [thread overview] Message-ID: <20110712084804.0235de42@endymion.delvare> (raw) In-Reply-To: <3737907.jTEM1bACya@bloomfield> On Mon, 11 Jul 2011 23:49:10 +0200, Pavel Herrmann wrote: > On Monday 11 of July 2011 14:03:13 Guenter Roeck wrote: > > On Mon, 2011-07-11 at 16:36 -0400, Pavel Herrmann wrote: > > > the structure is dynamically allocated, but the pointer used to hold it > > > is a static global var. > > > > This is true only if CONFIG_SHARPSL_PM is defined, and it assumes that > > the driver is instantiated exactly once. That is pretty badly broken > > (the commit introducing it even admits that), and should be fixed. This > > does not happen CONFIG_SHARPSL_PM is not defined. If CONFIG_SHARPSL_PM > > _is_ defined in your environment, and you do have multiple instances of > > the driver (ie if you have multiple MAX1111 chips in your system), a > > severe problem is that max1111_read_channel() does not identify the > > driver instance. That can not be fixed with a mutex. > > if you don't have CONFIG_SHARPSL_PM then there is nothing calling > max1111_read, and thus any of the discussed doesn't matter This assumption of yours is incorrect. Even with CONFIG_SHARPSL_PM disabled, the max1111 driver creates sysfs attributes which, when read, call max1111_read(). What CONFIG_SHARPSL_PM adds is the in-kernel access. > AFAIK max1111 is only used in sharpsl devices (according to kernel drivers > anyways), and only one a piece. > this patch is meant to fix a crash, not make the driver code pretty just in > case someone else decides to use it. this patch also doesn't present any more > challenges for solving the multiple devices issue and would be necessary > either way, as drvdata is not thread-safe anyways (or I am badly mistaken) You are right, drvdata is not thread-safe, and this is the most obvious reason why your patch is needed. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
next prev parent reply other threads:[~2011-07-12 6:48 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-07-11 18:47 [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-07-11 18:47 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing NULL Pavel Herrmann 2011-07-11 20:11 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Jean Delvare 2011-07-11 20:11 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Jean Delvare 2011-07-11 20:36 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-07-11 20:36 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Pavel Herrmann 2011-07-11 21:03 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Guenter Roeck 2011-07-11 21:03 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Guenter Roeck 2011-07-11 21:49 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-07-11 21:49 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Pavel Herrmann 2011-07-12 6:48 ` Jean Delvare [this message] 2011-07-12 6:48 ` Jean Delvare 2011-07-11 20:56 ` [Zaurus-devel] [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Stanislav Brabec 2011-07-11 20:56 ` [lm-sensors] [Zaurus-devel] [PATCH v2] MAX1111: Fix Race Stanislav Brabec -- strict thread matches above, loose matches on Subject: below -- 2011-06-03 20:00 [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-06-29 20:11 ` Pavel Herrmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110712084804.0235de42@endymion.delvare \ --to=khali@linux-fr.org \ --cc=linux-arm-kernel@lists.infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.