[v2,1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()
diff mbox

Message ID 1395143914-26929-1-git-send-email-tiwai@suse.de
State Not Applicable
Headers show

Commit Message

Takashi Iwai March 18, 2014, 11:58 a.m. UTC
regmap deploys the spinlock for the protection when set up in fast_io
mode.  This may lead to sleep-in-atomic by memory allocation with
GFP_KERNEL in regmap_bulk_write().  This patch fixes it by moving the
allocation out of the lock.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/regmap/regmap.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Mark Brown March 18, 2014, 12:22 p.m. UTC | #1
On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote:

>  		wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL);
>  		if (!wval) {
> -			ret = -ENOMEM;
>  			dev_err(map->dev, "Error in memory allocation\n");
> -			goto out;
> +			return -ENOMEM;
>  		}
> +		map->lock(map->lock_arg);
>  		for (i = 0; i < val_count * val_bytes; i += val_bytes)
>  			map->format.parse_inplace(wval + i);
>  
>  		ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count);
> +		map->unlock(map->lock_arg);

If we're reducing the locking region here then we should take the lock
after doing the parse_inplace() to reduce the locked region.  Nothing
else can be referring to the data since we only just allocated it.  I'll
fix that by hand and apply.

Please also send things to the list for the subsystem (linux-kernel if
there's not a specific one).
Takashi Iwai March 18, 2014, 12:57 p.m. UTC | #2
At Tue, 18 Mar 2014 12:22:18 +0000,
Mark Brown wrote:
> 
> On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote:
> 
> >  		wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL);
> >  		if (!wval) {
> > -			ret = -ENOMEM;
> >  			dev_err(map->dev, "Error in memory allocation\n");
> > -			goto out;
> > +			return -ENOMEM;
> >  		}
> > +		map->lock(map->lock_arg);
> >  		for (i = 0; i < val_count * val_bytes; i += val_bytes)
> >  			map->format.parse_inplace(wval + i);
> >  
> >  		ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count);
> > +		map->unlock(map->lock_arg);
> 
> If we're reducing the locking region here then we should take the lock
> after doing the parse_inplace() to reduce the locked region.  Nothing
> else can be referring to the data since we only just allocated it.  I'll
> fix that by hand and apply.

I thought of that, too, but didn't take it because covering the lock
there doesn't change the fact that it's still fundamentally racy.

> Please also send things to the list for the subsystem (linux-kernel if
> there's not a specific one).

OK, I just copied the previous recipient of the thread...


Takashi
Mark Brown March 18, 2014, 1:04 p.m. UTC | #3
On Tue, Mar 18, 2014 at 01:57:22PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote:

> > > +		map->lock(map->lock_arg);
> > >  		for (i = 0; i < val_count * val_bytes; i += val_bytes)
> > >  			map->format.parse_inplace(wval + i);
> > >  
> > >  		ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count);
> > > +		map->unlock(map->lock_arg);

> > If we're reducing the locking region here then we should take the lock
> > after doing the parse_inplace() to reduce the locked region.  Nothing
> > else can be referring to the data since we only just allocated it.  I'll
> > fix that by hand and apply.

> I thought of that, too, but didn't take it because covering the lock
> there doesn't change the fact that it's still fundamentally racy.

I'm not sure what you mean here - what do you mean yb "covering the
lock"?

> > Please also send things to the list for the subsystem (linux-kernel if
> > there's not a specific one).

> OK, I just copied the previous recipient of the thread...

Sure, but if there's another subsystem adding that subsystem helps
things along.
Takashi Iwai March 18, 2014, 1:36 p.m. UTC | #4
At Tue, 18 Mar 2014 13:04:26 +0000,
Mark Brown wrote:
> 
> On Tue, Mar 18, 2014 at 01:57:22PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote:
> 
> > > > +		map->lock(map->lock_arg);
> > > >  		for (i = 0; i < val_count * val_bytes; i += val_bytes)
> > > >  			map->format.parse_inplace(wval + i);
> > > >  
> > > >  		ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count);
> > > > +		map->unlock(map->lock_arg);
> 
> > > If we're reducing the locking region here then we should take the lock
> > > after doing the parse_inplace() to reduce the locked region.  Nothing
> > > else can be referring to the data since we only just allocated it.  I'll
> > > fix that by hand and apply.
> 
> > I thought of that, too, but didn't take it because covering the lock
> > there doesn't change the fact that it's still fundamentally racy.
> 
> I'm not sure what you mean here - what do you mean yb "covering the
> lock"?

I meant covering memcpy() and parse_inplace() & co in the lock.


Takashi
Mark Brown March 18, 2014, 8:21 p.m. UTC | #5
On Tue, Mar 18, 2014 at 02:36:53PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > > I thought of that, too, but didn't take it because covering the lock
> > > there doesn't change the fact that it's still fundamentally racy.

> > I'm not sure what you mean here - what do you mean yb "covering the
> > lock"?

> I meant covering memcpy() and parse_inplace() & co in the lock.

Oh, right.  A fix is definitely needed and your fix is certainly good
from a correctness point of view but since we're narrowing the locked
region we may as well make it as small as possible while we're at it
both for comprehensibility ("why is that locked?") and performance.

Patch
diff mbox

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6a19515f8a45..6d134a3cbfd3 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1520,12 +1520,12 @@  int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 	if (reg % map->reg_stride)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
 	/*
 	 * Some devices don't support bulk write, for
 	 * them we have a series of single write operations.
 	 */
 	if (!map->bus || map->use_single_rw) {
+		map->lock(map->lock_arg);
 		for (i = 0; i < val_count; i++) {
 			unsigned int ival;
 
@@ -1554,24 +1554,25 @@  int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 			if (ret != 0)
 				goto out;
 		}
+out:
+		map->unlock(map->lock_arg);
 	} else {
 		void *wval;
 
 		wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL);
 		if (!wval) {
-			ret = -ENOMEM;
 			dev_err(map->dev, "Error in memory allocation\n");
-			goto out;
+			return -ENOMEM;
 		}
+		map->lock(map->lock_arg);
 		for (i = 0; i < val_count * val_bytes; i += val_bytes)
 			map->format.parse_inplace(wval + i);
 
 		ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count);
+		map->unlock(map->lock_arg);
 
 		kfree(wval);
 	}
-out:
-	map->unlock(map->lock_arg);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regmap_bulk_write);