[alsa-devel] [PATCH v2 1/2] remgap: Fix possible sleep-in-atomic in regmap_bulk_write()
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(-)
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);
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@suse.de --- 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;
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.
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).
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
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.
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
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.
participants (2)
-
Mark Brown -
Takashi Iwai