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

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

[Takashi Iwai]

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-l3A5Bk7waGM@public.gmane.org>
---
 drivers/base/regmap/regmap.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

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);
-- 
1.9.0

[PATCH v2 2/2] remgap: Fix possible sleep-in-atomic in regmap_register_patch()

[Takashi Iwai]

Just like the previous, fix the possible sleep-in-atomic in
regmap_register_patch() by moving the allocation out of the lock.

This makes the function unsafe for concurrent access as map->patch and
map->patch_regs are changed without lock now.  But such a use case
must be very rare, thus we take rather simplicity over complexity, and
add a note in the function description mentioning this restriction.

Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 drivers/base/regmap/regmap.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6d134a3cbfd3..e6827cee29a3 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2174,6 +2174,10 @@ EXPORT_SYMBOL_GPL(regmap_async_complete);
  * apply them immediately.  Typically this is used to apply
  * corrections to be applied to the device defaults on startup, such
  * as the updates some vendors provide to undocumented registers.
+ *
+ * Note that the function gives no protection over concurrent access
+ * to map->patch and map->patch_regs.  If needed, apply some lock in
+ * the caller side.
  */
 int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 			  int num_regs)
@@ -2186,6 +2190,16 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 	    num_regs))
 		return 0;
 
+	p = krealloc(map->patch,
+		     sizeof(struct reg_default) * (map->patch_regs + num_regs),
+		     GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs));
+	map->patch = p;
+	map->patch_regs += num_regs;
+
 	map->lock(map->lock_arg);
 
 	bypass = map->cache_bypass;
@@ -2203,17 +2217,6 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 		}
 	}
 
-	p = krealloc(map->patch,
-		     sizeof(struct reg_default) * (map->patch_regs + num_regs),
-		     GFP_KERNEL);
-	if (p) {
-		memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs));
-		map->patch = p;
-		map->patch_regs += num_regs;
-	} else {
-		ret = -ENOMEM;
-	}
-
 out:
 	map->async = false;
 	map->cache_bypass = bypass;
-- 
1.9.0

Re: [PATCH v2 2/2] remgap: Fix possible sleep-in-atomic in regmap_register_patch()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

On Tue, Mar 18, 2014 at 12:58:34PM +0100, Takashi Iwai wrote:
> Just like the previous, fix the possible sleep-in-atomic in
> regmap_register_patch() by moving the allocation out of the lock.

Sorry, I guess I wasn't clear - I already have essentially the same
patch locally.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

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).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()

[Takashi Iwai]

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

Re: [PATCH v2 1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()

[Takashi Iwai]

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

Re: [PATCH v2 1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]