[PATCH for v3.15 0/2] UBI: block: Support very large volumes

[PATCH for v3.15 0/2] UBI: block: Support very large volumes

[Ezequiel Garcia]

This is a re-spin of Richard's work [1]. I've added a patch for some minor
clean-up, so the large volume fix is a less intrusive one.

The proposed returned value E2BIG has been changed for EFBIG, which results
in a slightly more accurate error. Also, the user messages have been removed
entirely; returning an error seems enough.

This bug was reported on #mtd IRC. I don't have to reporter name to give proper
credit. It should be applied as a fix for v3.15.

Richard: Thanks a lot for hunting this down, and feel free to disagree about
my changes.

[1] https://lkml.org/lkml/2014/3/19/333
 
Ezequiel Garcia (1):
  UBI: block: Set disk_capacity out of the mutex

Richard Weinberger (1):
  UBI: block: Avoid disk size integer overflow

 drivers/mtd/ubi/block.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 1/2] UBI: block: Set disk_capacity out of the mutex

[Ezequiel Garcia]

There's no need to set the disk capacity with the mutex held, so this
commit takes the variable setting out of the mutex. This simplifies
the disk capacity fix for very large volumes in a follow up commit.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 8d659e6..ad2cf78 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -378,7 +378,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
 	struct gendisk *gd;
-	int disk_capacity;
+	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 	int ret;
 
 	/* Check that the volume isn't already handled */
@@ -412,7 +412,6 @@ int ubiblock_create(struct ubi_volume_info *vi)
 	gd->first_minor = dev->ubi_num * UBI_MAX_VOLUMES + dev->vol_id;
 	gd->private_data = dev;
 	sprintf(gd->disk_name, "ubiblock%d_%d", dev->ubi_num, dev->vol_id);
-	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 	set_capacity(gd, disk_capacity);
 	dev->gd = gd;
 
@@ -499,7 +498,7 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 static void ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
-	int disk_capacity;
+	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 
 	/*
 	 * Need to lock the device list until we stop using the device,
@@ -514,7 +513,6 @@ static void ubiblock_resize(struct ubi_volume_info *vi)
 	}
 
 	mutex_lock(&dev->dev_mutex);
-	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
 	set_capacity(dev->gd, disk_capacity);
 	ubi_msg("%s resized to %d LEBs", dev->gd->disk_name, vi->size);
 	mutex_unlock(&dev->dev_mutex);
-- 
1.9.1

[PATCH 2/2] UBI: block: Avoid disk size integer overflow

[Ezequiel Garcia]

From: Richard Weinberger <richard@nod.at>

This patch fixes the issue that on very large UBI volumes
UBI block does not work correctly.

Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/block.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index ad2cf78..4cff2f1 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -378,9 +378,11 @@ int ubiblock_create(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
 	struct gendisk *gd;
-	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	u64 disk_capacity = ((u64)vi->size * vi->usable_leb_size) >> 9;
 	int ret;
 
+	if ((sector_t)disk_capacity != disk_capacity)
+		return -EFBIG;
 	/* Check that the volume isn't already handled */
 	mutex_lock(&devices_mutex);
 	if (find_dev_nolock(vi->ubi_num, vi->vol_id)) {
@@ -498,8 +500,10 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 static void ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
-	int disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	u64 disk_capacity = ((u64)vi->size * vi->usable_leb_size) >> 9;
 
+	if ((sector_t)disk_capacity != disk_capacity)
+		return -EFBIG;
 	/*
 	 * Need to lock the device list until we stop using the device,
 	 * otherwise the device struct might get released in
-- 
1.9.1

Re: [PATCH for v3.15 0/2] UBI: block: Support very large volumes

[Ezequiel Garcia]

Artem,

On Apr 17, Ezequiel Garcia wrote:
> This is a re-spin of Richard's work [1]. I've added a patch for some minor
> clean-up, so the large volume fix is a less intrusive one.
> 
> The proposed returned value E2BIG has been changed for EFBIG, which results
> in a slightly more accurate error. Also, the user messages have been removed
> entirely; returning an error seems enough.
> 
> This bug was reported on #mtd IRC. I don't have to reporter name to give proper
> credit. It should be applied as a fix for v3.15.
> 
> Richard: Thanks a lot for hunting this down, and feel free to disagree about
> my changes.
> 
> [1] https://lkml.org/lkml/2014/3/19/333
>  
> Ezequiel Garcia (1):
>   UBI: block: Set disk_capacity out of the mutex
> 
> Richard Weinberger (1):
>   UBI: block: Avoid disk size integer overflow
> 
>  drivers/mtd/ubi/block.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

Do you think you can pick this for v3.15-rc3?

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH for v3.15 0/2] UBI: block: Support very large volumes

[Artem Bityutskiy]

On Wed, 2014-04-23 at 20:59 -0300, Ezequiel Garcia wrote:
> >  drivers/mtd/ubi/block.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> 
> Do you think you can pick this for v3.15-rc3?

Picked both, I'll send them to Linus soon, thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH for v3.15 0/2] UBI: block: Support very large volumes

[Artem Bityutskiy]

On Mon, 2014-05-05 at 09:36 +0300, Artem Bityutskiy wrote:
> On Wed, 2014-04-23 at 20:59 -0300, Ezequiel Garcia wrote:
> > >  drivers/mtd/ubi/block.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > 
> > Do you think you can pick this for v3.15-rc3?
> 
> Picked both, I'll send them to Linus soon, thanks!

So kbuild caught a warning. Shame I did not notice this myself, but the
warning indicates a bigger problem - the entire function is not allowed
to fail, but it seems like there are conditions when it needs to fail.
Would you please analyze the error handling in the resize path?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH for v3.15 0/2] UBI: block: Support very large volumes

[Ezequiel Garcia]

On 05 May 10:23 AM, Artem Bityutskiy wrote:
> On Mon, 2014-05-05 at 09:36 +0300, Artem Bityutskiy wrote:
> > On Wed, 2014-04-23 at 20:59 -0300, Ezequiel Garcia wrote:
> > > >  drivers/mtd/ubi/block.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > 
> > > Do you think you can pick this for v3.15-rc3?
> > 
> > Picked both, I'll send them to Linus soon, thanks!
> 
> So kbuild caught a warning. Shame I did not notice this myself, but the
> warning indicates a bigger problem - the entire function is not allowed
> to fail, but it seems like there are conditions when it needs to fail.
> Would you please analyze the error handling in the resize path?
> 

Working on it.

Sorry for the crap,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH for v3.15 0/2] UBI: block: Support very large volumes

[Ezequiel Garcia]

On 05 May 10:23 AM, Artem Bityutskiy wrote:
> On Mon, 2014-05-05 at 09:36 +0300, Artem Bityutskiy wrote:
> > On Wed, 2014-04-23 at 20:59 -0300, Ezequiel Garcia wrote:
> > > >  drivers/mtd/ubi/block.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > 
> > > Do you think you can pick this for v3.15-rc3?
> > 
> > Picked both, I'll send them to Linus soon, thanks!
> 
> So kbuild caught a warning. Shame I did not notice this myself, but the
> warning indicates a bigger problem - the entire function is not allowed
> to fail, but it seems like there are conditions when it needs to fail.
> Would you please analyze the error handling in the resize path?
> 

The ubiblock_resize function fails if the number of 512B sectors are not
expressable as a sector_t. This function is only used to resize the block
device, *after* UBI has resized the associated volume.

In other words, the UBI block driver fails to acknowledge the UBI volume resize
if the new size is larger than the supported Linux block size (2 TiB if
CONFIG_LBDAF=n, or a lot more if CONFIG_LBDAF=y).

I don't think there's anything we can do, except maybe print a warning.

I'll push a v2 with this in mind.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com