[PATCH 0/4] Fix various issues with RFD and FTLs

[PATCH 0/4] Fix various issues with RFD and FTLs

[Sean Young]

Sean Young (4):
  mtd: rfd_ftl: allow use of MTD_RAM for testing purposes
  mtd: rfd_ftl: add discard support
  mtd: blk_devs: make discard work on FTLs
  mtd: rfd_ftl: fix use-after-free

 drivers/mtd/mtd_blkdevs.c |  1 +
 drivers/mtd/rfd_ftl.c     | 44 ++++++++++++++++++++++++++++++++-------
 2 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/4] mtd: rfd_ftl: allow use of MTD_RAM for testing purposes

[Sean Young]

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/mtd/rfd_ftl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
index 6e0d5ce9b010..7b243f2b2fa3 100644
--- a/drivers/mtd/rfd_ftl.c
+++ b/drivers/mtd/rfd_ftl.c
@@ -720,7 +720,8 @@ static void rfd_ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 {
 	struct partition *part;
 
-	if (mtd->type != MTD_NORFLASH || mtd->size > UINT_MAX)
+	if ((mtd->type != MTD_NORFLASH && mtd->type != MTD_RAM) ||
+	    mtd->size > UINT_MAX)
 		return;
 
 	part = kzalloc(sizeof(struct partition), GFP_KERNEL);
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/4] mtd: rfd_ftl: add discard support

[Sean Young]

I proposed this change 16 years ago before discard was a feature in
the block layer: https://lwn.net/Articles/162776/

Now that the block layer has discard, we can finally merge this change.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/mtd/rfd_ftl.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
index 7b243f2b2fa3..7f5f6d247cae 100644
--- a/drivers/mtd/rfd_ftl.c
+++ b/drivers/mtd/rfd_ftl.c
@@ -705,6 +705,34 @@ static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *
 	return rc;
 }
 
+static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev,
+			       unsigned long sector, unsigned int nr_sects)
+{
+	struct partition *part = (struct partition *)dev;
+	u_long addr;
+	int rc;
+
+	while (nr_sects) {
+		if (sector >= part->sector_count)
+			return -EIO;
+
+		addr = part->sector_map[sector];
+
+		if (addr != -1) {
+			rc = mark_sector_deleted(part, addr);
+			if (rc)
+				return rc;
+
+			part->sector_map[sector] = -1;
+		}
+
+		sector++;
+		nr_sects--;
+	}
+
+	return 0;
+}
+
 static int rfd_ftl_getgeo(struct mtd_blktrans_dev *dev, struct hd_geometry *geo)
 {
 	struct partition *part = (struct partition*)dev;
@@ -786,6 +814,7 @@ static struct mtd_blktrans_ops rfd_ftl_tr = {
 
 	.readsect	= rfd_ftl_readsect,
 	.writesect	= rfd_ftl_writesect,
+	.discard	= rfd_ftl_discardsect,
 	.getgeo		= rfd_ftl_getgeo,
 	.add_mtd	= rfd_ftl_add_mtd,
 	.remove_dev	= rfd_ftl_remove_dev,
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/4] mtd: blk_devs: make discard work on FTLs

[Sean Young]

Without this change, any discard ioctl fails with -ENOTSUPP and the
following logged:

rfda: Error: discard_granularity is 0.
rfda: Error: discard_granularity is 0.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/mtd/mtd_blkdevs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 6ce4bc57f919..486251e058a6 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -419,6 +419,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	if (tr->discard) {
 		blk_queue_flag_set(QUEUE_FLAG_DISCARD, new->rq);
 		blk_queue_max_discard_sectors(new->rq, UINT_MAX);
+		new->rq->limits.discard_granularity = 512;
 	}
 
 	gd->queue = new->rq;
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/4] mtd: rfd_ftl: fix use-after-free

[Sean Young]

del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use
container_of() to make it clearer what the cast is doing.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/mtd/rfd_ftl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
index 7f5f6d247cae..af20a0a71108 100644
--- a/drivers/mtd/rfd_ftl.c
+++ b/drivers/mtd/rfd_ftl.c
@@ -239,7 +239,7 @@ static int scan_header(struct partition *part)
 
 static int rfd_ftl_readsect(struct mtd_blktrans_dev *dev, u_long sector, char *buf)
 {
-	struct partition *part = (struct partition*)dev;
+	struct partition *part = container_of(dev, struct partition, mbd);
 	u_long addr;
 	size_t retlen;
 	int rc;
@@ -600,7 +600,7 @@ static int find_free_sector(const struct partition *part, const struct block *bl
 
 static int do_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *buf, ulong *old_addr)
 {
-	struct partition *part = (struct partition*)dev;
+	struct partition *part = container_of(dev, struct partition, mbd);
 	struct block *block;
 	u_long addr;
 	int i;
@@ -666,7 +666,7 @@ static int do_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *buf,
 
 static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *buf)
 {
-	struct partition *part = (struct partition*)dev;
+	struct partition *part = container_of(dev, struct partition, mbd);
 	u_long old_addr;
 	int i;
 	int rc = 0;
@@ -708,7 +708,7 @@ static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *
 static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev,
 			       unsigned long sector, unsigned int nr_sects)
 {
-	struct partition *part = (struct partition *)dev;
+	struct partition *part = container_of(dev, struct partition, mbd);
 	u_long addr;
 	int rc;
 
@@ -735,7 +735,7 @@ static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev,
 
 static int rfd_ftl_getgeo(struct mtd_blktrans_dev *dev, struct hd_geometry *geo)
 {
-	struct partition *part = (struct partition*)dev;
+	struct partition *part = container_of(dev, struct partition, mbd);
 
 	geo->heads = 1;
 	geo->sectors = SECTORS_PER_TRACK;
@@ -792,7 +792,7 @@ static void rfd_ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 
 static void rfd_ftl_remove_dev(struct mtd_blktrans_dev *dev)
 {
-	struct partition *part = (struct partition*)dev;
+	struct partition *part = container_of(dev, struct partition, mbd);
 	int i;
 
 	for (i=0; i<part->total_blocks; i++) {
@@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct mtd_blktrans_dev *dev)
 			part->mbd.mtd->name, i, part->blocks[i].erases);
 	}
 
-	del_mtd_blktrans_dev(dev);
 	vfree(part->sector_map);
 	kfree(part->header_cache);
 	kfree(part->blocks);
+	del_mtd_blktrans_dev(&part->mbd);
 }
 
 static struct mtd_blktrans_ops rfd_ftl_tr = {
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/4] Fix various issues with RFD and FTLs

[Sean Young]

Hi Miquel, Richard, Vignesh,

On Tue, Jul 13, 2021 at 10:43:59AM +0100, Sean Young wrote:
> Sean Young (4):
>   mtd: rfd_ftl: allow use of MTD_RAM for testing purposes
>   mtd: rfd_ftl: add discard support
>   mtd: blk_devs: make discard work on FTLs
>   mtd: rfd_ftl: fix use-after-free
> 
>  drivers/mtd/mtd_blkdevs.c |  1 +
>  drivers/mtd/rfd_ftl.c     | 44 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 38 insertions(+), 7 deletions(-)

Do the mtd maintainers have any feedback on this patch series, or will
they be accepted?

Many thanks,

Sean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] mtd: rfd_ftl: allow use of MTD_RAM for testing purposes

[Miquel Raynal]

Hi Sean,

Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:00 +0100:

You miss a commit message here.

Is this a real patch or just something for debugging purpose that we
should keep out of tree? I don't really see the reason for this patch
even though I am not strongly opposed to it.

> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/mtd/rfd_ftl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> index 6e0d5ce9b010..7b243f2b2fa3 100644
> --- a/drivers/mtd/rfd_ftl.c
> +++ b/drivers/mtd/rfd_ftl.c
> @@ -720,7 +720,8 @@ static void rfd_ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
>  {
>  	struct partition *part;
>  
> -	if (mtd->type != MTD_NORFLASH || mtd->size > UINT_MAX)
> +	if ((mtd->type != MTD_NORFLASH && mtd->type != MTD_RAM) ||
> +	    mtd->size > UINT_MAX)
>  		return;
>  
>  	part = kzalloc(sizeof(struct partition), GFP_KERNEL);

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/4] mtd: rfd_ftl: add discard support

[Miquel Raynal]

Hi Sean,

Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:01 +0100:

> I proposed this change 16 years ago before discard was a feature in
> the block layer: https://lwn.net/Articles/162776/
> 
> Now that the block layer has discard, we can finally merge this change.

Can you explain why this is needed here?

(also please add the maintainers in Cc: for the v2)

> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/mtd/rfd_ftl.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> index 7b243f2b2fa3..7f5f6d247cae 100644
> --- a/drivers/mtd/rfd_ftl.c
> +++ b/drivers/mtd/rfd_ftl.c
> @@ -705,6 +705,34 @@ static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *
>  	return rc;
>  }
>  
> +static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev,
> +			       unsigned long sector, unsigned int nr_sects)
> +{
> +	struct partition *part = (struct partition *)dev;
> +	u_long addr;
> +	int rc;
> +
> +	while (nr_sects) {
> +		if (sector >= part->sector_count)
> +			return -EIO;
> +
> +		addr = part->sector_map[sector];
> +
> +		if (addr != -1) {
> +			rc = mark_sector_deleted(part, addr);
> +			if (rc)
> +				return rc;
> +
> +			part->sector_map[sector] = -1;
> +		}
> +
> +		sector++;
> +		nr_sects--;
> +	}
> +
> +	return 0;
> +}
> +
>  static int rfd_ftl_getgeo(struct mtd_blktrans_dev *dev, struct hd_geometry *geo)
>  {
>  	struct partition *part = (struct partition*)dev;
> @@ -786,6 +814,7 @@ static struct mtd_blktrans_ops rfd_ftl_tr = {
>  
>  	.readsect	= rfd_ftl_readsect,
>  	.writesect	= rfd_ftl_writesect,
> +	.discard	= rfd_ftl_discardsect,
>  	.getgeo		= rfd_ftl_getgeo,
>  	.add_mtd	= rfd_ftl_add_mtd,
>  	.remove_dev	= rfd_ftl_remove_dev,




Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rfd_ftl: fix use-after-free

[Miquel Raynal]

Hi Sean,

Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100:

> del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use
> container_of() to make it clearer what the cast is doing.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/mtd/rfd_ftl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> index 7f5f6d247cae..af20a0a71108 100644
> --- a/drivers/mtd/rfd_ftl.c
> +++ b/drivers/mtd/rfd_ftl.c

[...]

> @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct
mtd_blktrans_dev *dev)
>  			part->mbd.mtd->name, i, part->blocks[i].erases);
>  	}
>  
> -	del_mtd_blktrans_dev(dev);
>  	vfree(part->sector_map);
>  	kfree(part->header_cache);
>  	kfree(part->blocks);
> +	del_mtd_blktrans_dev(&part->mbd);

I am not sure moving this call at the bottom of ftl_remove_dev makes
sense, can we keep it where it was and just do the s/dev/part->mbd/ ?

>  }
>  
>  static struct mtd_blktrans_ops rfd_ftl_tr = {

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] mtd: rfd_ftl: allow use of MTD_RAM for testing purposes

[Sean Young]

On Fri, Aug 06, 2021 at 08:16:29PM +0200, Miquel Raynal wrote:
> Hi Sean,
> 
> Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:00 +0100:
> 
> You miss a commit message here.
> 
> Is this a real patch or just something for debugging purpose that we
> should keep out of tree? I don't really see the reason for this patch
> even though I am not strongly opposed to it.

This allows the mtdram module to be used as a test bed. This means
different mtd sizes can be tested and it can be tested on platforms
without an mtd device.

I'll add a better commit message for v2.

Thanks for the review

Sean

> 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/mtd/rfd_ftl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> > index 6e0d5ce9b010..7b243f2b2fa3 100644
> > --- a/drivers/mtd/rfd_ftl.c
> > +++ b/drivers/mtd/rfd_ftl.c
> > @@ -720,7 +720,8 @@ static void rfd_ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
> >  {
> >  	struct partition *part;
> >  
> > -	if (mtd->type != MTD_NORFLASH || mtd->size > UINT_MAX)
> > +	if ((mtd->type != MTD_NORFLASH && mtd->type != MTD_RAM) ||
> > +	    mtd->size > UINT_MAX)
> >  		return;
> >  
> >  	part = kzalloc(sizeof(struct partition), GFP_KERNEL);
> 
> Thanks,
> Miquèl
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rfd_ftl: fix use-after-free

[Sean Young]

On Fri, Aug 06, 2021 at 08:21:58PM +0200, Miquel Raynal wrote:
> Hi Sean,
> 
> Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100:
> 
> > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use
> > container_of() to make it clearer what the cast is doing.
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/mtd/rfd_ftl.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> > index 7f5f6d247cae..af20a0a71108 100644
> > --- a/drivers/mtd/rfd_ftl.c
> > +++ b/drivers/mtd/rfd_ftl.c
> 
> [...]
> 
> > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct
> mtd_blktrans_dev *dev)
> >  			part->mbd.mtd->name, i, part->blocks[i].erases);
> >  	}
> >  
> > -	del_mtd_blktrans_dev(dev);
> >  	vfree(part->sector_map);
> >  	kfree(part->header_cache);
> >  	kfree(part->blocks);
> > +	del_mtd_blktrans_dev(&part->mbd);
> 
> I am not sure moving this call at the bottom of ftl_remove_dev makes
> sense, can we keep it where it was and just do the s/dev/part->mbd/ ?

The reason for this patch is that del_mtd_blktrans_dev() kfrees its argument,
so both part and dev point to freed memory. This means it's a use after free.

Thanks,

Sean

> 
> >  }
> >  
> >  static struct mtd_blktrans_ops rfd_ftl_tr = {
> 
> Thanks,
> Miquèl
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/4] mtd: rfd_ftl: add discard support

[Sean Young]

On Fri, Aug 06, 2021 at 08:18:20PM +0200, Miquel Raynal wrote:
> Hi Sean,
> 
> Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:01 +0100:
> 
> > I proposed this change 16 years ago before discard was a feature in
> > the block layer: https://lwn.net/Articles/162776/
> > 
> > Now that the block layer has discard, we can finally merge this change.
> 
> Can you explain why this is needed here?

Because discard is a huge win. This is also known as trim.

By implementing discard, both fstrim and the discard filesystem option
can be used.

Implementing discard in the ftl means that when files are removed, there
is less data in the ftl mapping. This means less stuff to move around for
erasing and also less erasing to do. This means better wear levelling.

There is also a wikipedia page on trim here:

	https://en.wikipedia.org/wiki/Trim_(computing)

Thanks,

Sean

> (also please add the maintainers in Cc: for the v2)
> 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/mtd/rfd_ftl.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> > index 7b243f2b2fa3..7f5f6d247cae 100644
> > --- a/drivers/mtd/rfd_ftl.c
> > +++ b/drivers/mtd/rfd_ftl.c
> > @@ -705,6 +705,34 @@ static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *
> >  	return rc;
> >  }
> >  
> > +static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev,
> > +			       unsigned long sector, unsigned int nr_sects)
> > +{
> > +	struct partition *part = (struct partition *)dev;
> > +	u_long addr;
> > +	int rc;
> > +
> > +	while (nr_sects) {
> > +		if (sector >= part->sector_count)
> > +			return -EIO;
> > +
> > +		addr = part->sector_map[sector];
> > +
> > +		if (addr != -1) {
> > +			rc = mark_sector_deleted(part, addr);
> > +			if (rc)
> > +				return rc;
> > +
> > +			part->sector_map[sector] = -1;
> > +		}
> > +
> > +		sector++;
> > +		nr_sects--;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int rfd_ftl_getgeo(struct mtd_blktrans_dev *dev, struct hd_geometry *geo)
> >  {
> >  	struct partition *part = (struct partition*)dev;
> > @@ -786,6 +814,7 @@ static struct mtd_blktrans_ops rfd_ftl_tr = {
> >  
> >  	.readsect	= rfd_ftl_readsect,
> >  	.writesect	= rfd_ftl_writesect,
> > +	.discard	= rfd_ftl_discardsect,
> >  	.getgeo		= rfd_ftl_getgeo,
> >  	.add_mtd	= rfd_ftl_add_mtd,
> >  	.remove_dev	= rfd_ftl_remove_dev,
> 
> 
> 
> 
> Thanks,
> Miquèl
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rfd_ftl: fix use-after-free

[Miquel Raynal]

Hi Sean,

Sean Young <sean@mess.org> wrote on Sat, 7 Aug 2021 08:57:35 +0100:

> On Fri, Aug 06, 2021 at 08:21:58PM +0200, Miquel Raynal wrote:
> > Hi Sean,
> > 
> > Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100:
> >   
> > > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use
> > > container_of() to make it clearer what the cast is doing.
> > > 
> > > Signed-off-by: Sean Young <sean@mess.org>
> > > ---
> > >  drivers/mtd/rfd_ftl.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> > > index 7f5f6d247cae..af20a0a71108 100644
> > > --- a/drivers/mtd/rfd_ftl.c
> > > +++ b/drivers/mtd/rfd_ftl.c  
> > 
> > [...]
> >   
> > > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct  
> > mtd_blktrans_dev *dev)  
> > >  			part->mbd.mtd->name, i, part->blocks[i].erases);
> > >  	}
> > >  
> > > -	del_mtd_blktrans_dev(dev);
> > >  	vfree(part->sector_map);
> > >  	kfree(part->header_cache);
> > >  	kfree(part->blocks);
> > > +	del_mtd_blktrans_dev(&part->mbd);  
> > 
> > I am not sure moving this call at the bottom of ftl_remove_dev makes
> > sense, can we keep it where it was and just do the s/dev/part->mbd/ ?  
> 
> The reason for this patch is that del_mtd_blktrans_dev() kfrees its argument,
> so both part and dev point to freed memory. This means it's a use after free.

Ok, please split this into two patches and we'll be good.
 
Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/4] mtd: rfd_ftl: add discard support

[Miquel Raynal]

Hi Sean,

Sean Young <sean@mess.org> wrote on Sat, 7 Aug 2021 09:06:33 +0100:

> On Fri, Aug 06, 2021 at 08:18:20PM +0200, Miquel Raynal wrote:
> > Hi Sean,
> > 
> > Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:01 +0100:
> >   
> > > I proposed this change 16 years ago before discard was a feature in
> > > the block layer: https://lwn.net/Articles/162776/
> > > 
> > > Now that the block layer has discard, we can finally merge this change.  
> > 
> > Can you explain why this is needed here?  
> 
> Because discard is a huge win. This is also known as trim.
> 
> By implementing discard, both fstrim and the discard filesystem option
> can be used.
> 
> Implementing discard in the ftl means that when files are removed, there
> is less data in the ftl mapping. This means less stuff to move around for
> erasing and also less erasing to do. This means better wear levelling.

And this is a good commit message for v2 :-)

> 
> There is also a wikipedia page on trim here:
> 
> 	https://en.wikipedia.org/wiki/Trim_(computing)

Got it, thanks!

> 
> Thanks,
> 
> Sean
> 
> > (also please add the maintainers in Cc: for the v2)
> >   
> > > Signed-off-by: Sean Young <sean@mess.org>
> > > ---
> > >  drivers/mtd/rfd_ftl.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> > > index 7b243f2b2fa3..7f5f6d247cae 100644
> > > --- a/drivers/mtd/rfd_ftl.c
> > > +++ b/drivers/mtd/rfd_ftl.c
> > > @@ -705,6 +705,34 @@ static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *
> > >  	return rc;
> > >  }
> > >  
> > > +static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev,
> > > +			       unsigned long sector, unsigned int nr_sects)
> > > +{
> > > +	struct partition *part = (struct partition *)dev;
> > > +	u_long addr;
> > > +	int rc;
> > > +
> > > +	while (nr_sects) {
> > > +		if (sector >= part->sector_count)
> > > +			return -EIO;
> > > +
> > > +		addr = part->sector_map[sector];
> > > +
> > > +		if (addr != -1) {
> > > +			rc = mark_sector_deleted(part, addr);
> > > +			if (rc)
> > > +				return rc;
> > > +
> > > +			part->sector_map[sector] = -1;
> > > +		}
> > > +
> > > +		sector++;
> > > +		nr_sects--;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int rfd_ftl_getgeo(struct mtd_blktrans_dev *dev, struct hd_geometry *geo)
> > >  {
> > >  	struct partition *part = (struct partition*)dev;
> > > @@ -786,6 +814,7 @@ static struct mtd_blktrans_ops rfd_ftl_tr = {
> > >  
> > >  	.readsect	= rfd_ftl_readsect,
> > >  	.writesect	= rfd_ftl_writesect,
> > > +	.discard	= rfd_ftl_discardsect,
> > >  	.getgeo		= rfd_ftl_getgeo,
> > >  	.add_mtd	= rfd_ftl_add_mtd,
> > >  	.remove_dev	= rfd_ftl_remove_dev,  
> > 
> > 
> > 
> > 
> > Thanks,
> > Miquèl
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/  




Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rfd_ftl: fix use-after-free

[Sean Young]

Hi Miquel,

On Sat, Aug 07, 2021 at 12:34:09PM +0200, Miquel Raynal wrote:
> Hi Sean,
> 
> Sean Young <sean@mess.org> wrote on Sat, 7 Aug 2021 08:57:35 +0100:
> 
> > On Fri, Aug 06, 2021 at 08:21:58PM +0200, Miquel Raynal wrote:
> > > Hi Sean,
> > > 
> > > Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100:
> > >   
> > > > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use
> > > > container_of() to make it clearer what the cast is doing.
> > > > 
> > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > ---
> > > >  drivers/mtd/rfd_ftl.c | 14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
> > > > index 7f5f6d247cae..af20a0a71108 100644
> > > > --- a/drivers/mtd/rfd_ftl.c
> > > > +++ b/drivers/mtd/rfd_ftl.c  
> > > 
> > > [...]
> > >   
> > > > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct  
> > > mtd_blktrans_dev *dev)  
> > > >  			part->mbd.mtd->name, i, part->blocks[i].erases);
> > > >  	}
> > > >  
> > > > -	del_mtd_blktrans_dev(dev);
> > > >  	vfree(part->sector_map);
> > > >  	kfree(part->header_cache);
> > > >  	kfree(part->blocks);
> > > > +	del_mtd_blktrans_dev(&part->mbd);  
> > > 
> > > I am not sure moving this call at the bottom of ftl_remove_dev makes
> > > sense, can we keep it where it was and just do the s/dev/part->mbd/ ?  
> > 
> > The reason for this patch is that del_mtd_blktrans_dev() kfrees its argument,
> > so both part and dev point to freed memory. This means it's a use after free.
> 
> Ok, please split this into two patches and we'll be good.

Good point.

Thank you for the review.

I'll send out v2 shortly.

Sean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/