| Message ID | 20180108211542.11891-4-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: Preparation patches for the SPI NAND framework | expand |
Hello Boris, On Mon, 8 Jan 2018 22:15:42 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Some of the check done in custom ->_read/write_oob() implementation > are already done by the core (in mtd_check_oob_ops()). Not sure this is relevant here as your series introduces changes for the SPI NAND framework, but there are other places where these checks are, IMHO, also redundant and could be removed. The "past end" string when grepped in the MTD folder core returns a few more hits. In the NAND core: - nand_do_read_oob() - nand_read_oob() - nand_do_write_oob() - nand_write_oob() Maybe also in onenand_base.c, but I am less confident for this one: - onenand_bbt_read_oob() What do you think? Thanks, Miquèl > > Suggested-by: Peter Pan <peterpansjtu@gmail.com> > [Remove redundant checks done in mtdpart.c] > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > Changes in v4: > - integrated into the SPI NAND preparation patchset > > Changes in v3: > - Rebased on mtd/next after "mtd: Stop directly calling master > ->_xxx() hooks from mtdpart code" had been dropped > > Changes in v2: > - Merge "mtd: Remove unneeded checks in part_{read,write}_oob()" into > this commit > --- > drivers/mtd/devices/docg3.c | 5 ----- > drivers/mtd/mtdpart.c | 23 ----------------------- > drivers/mtd/nand/nand_base.c | 31 > ------------------------------- drivers/mtd/onenand/onenand_base.c | > 18 ------------------ 4 files changed, 77 deletions(-) > > diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c > index 5fb5e93d1547..a85af236b44d 100644 > --- a/drivers/mtd/devices/docg3.c > +++ b/drivers/mtd/devices/docg3.c > @@ -904,9 +904,6 @@ static int doc_read_oob(struct mtd_info *mtd, > loff_t from, if (ooblen % DOC_LAYOUT_OOB_SIZE) > return -EINVAL; > > - if (from + len > mtd->size) > - return -EINVAL; > - > ops->oobretlen = 0; > ops->retlen = 0; > ret = 0; > @@ -1441,8 +1438,6 @@ static int doc_write_oob(struct mtd_info *mtd, > loff_t ofs, if (len && ooblen && > (len / DOC_LAYOUT_PAGE_SIZE) != (ooblen / oobdelta)) > return -EINVAL; > - if (ofs + len > mtd->size) > - return -EINVAL; > > ops->oobretlen = 0; > ops->retlen = 0; > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index 07ef168ded98..131bb1d52871 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -108,25 +108,6 @@ static int part_read_oob(struct mtd_info *mtd, > loff_t from, struct mtd_ecc_stats stats; > int res; > > - if (from >= mtd->size) > - return -EINVAL; > - if (ops->datbuf && from + ops->len > mtd->size) > - return -EINVAL; > - > - /* > - * If OOB is also requested, make sure that we do not read > past the end > - * of this partition. > - */ > - if (ops->oobbuf) { > - size_t len, pages; > - > - len = mtd_oobavail(mtd, ops); > - pages = mtd_div_by_ws(mtd->size, mtd); > - pages -= mtd_div_by_ws(from, mtd); > - if (ops->ooboffs + ops->ooblen > pages * len) > - return -EINVAL; > - } > - > res = part->parent->_read_oob(part->parent, from + > part->offset, ops); if (unlikely(mtd_is_eccerr(res))) > mtd->ecc_stats.failed += > @@ -190,10 +171,6 @@ static int part_write_oob(struct mtd_info *mtd, > loff_t to, { > struct mtd_part *part = mtd_to_part(mtd); > > - if (to >= mtd->size) > - return -EINVAL; > - if (ops->datbuf && to + ops->len > mtd->size) > - return -EINVAL; > return part->parent->_write_oob(part->parent, to + > part->offset, ops); } > > diff --git a/drivers/mtd/nand/nand_base.c > b/drivers/mtd/nand/nand_base.c index 889ceadbf607..61eeac233683 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2187,21 +2187,6 @@ static int nand_do_read_oob(struct mtd_info > *mtd, loff_t from, > len = mtd_oobavail(mtd, ops); > > - if (unlikely(ops->ooboffs >= len)) { > - pr_debug("%s: attempt to start read outside oob\n", > - __func__); > - return -EINVAL; > - } > - > - /* Do not allow reads past end of device */ > - if (unlikely(from >= mtd->size || > - ops->ooboffs + readlen > ((mtd->size >> > chip->page_shift) - > - (from >> chip->page_shift)) > * len)) { > - pr_debug("%s: attempt to read beyond end of > device\n", > - __func__); > - return -EINVAL; > - } > - > chipnr = (int)(from >> chip->chip_shift); > chip->select_chip(mtd, chipnr); > > @@ -2820,22 +2805,6 @@ static int nand_do_write_oob(struct mtd_info > *mtd, loff_t to, return -EINVAL; > } > > - if (unlikely(ops->ooboffs >= len)) { > - pr_debug("%s: attempt to start write outside oob\n", > - __func__); > - return -EINVAL; > - } > - > - /* Do not allow write past end of device */ > - if (unlikely(to >= mtd->size || > - ops->ooboffs + ops->ooblen > > - ((mtd->size >> chip->page_shift) - > - (to >> chip->page_shift)) * len)) { > - pr_debug("%s: attempt to write beyond end of > device\n", > - __func__); > - return -EINVAL; > - } > - > chipnr = (int)(to >> chip->chip_shift); > > /* > diff --git a/drivers/mtd/onenand/onenand_base.c > b/drivers/mtd/onenand/onenand_base.c index 050ba8a87543..979f4031f23c > 100644 --- a/drivers/mtd/onenand/onenand_base.c > +++ b/drivers/mtd/onenand/onenand_base.c > @@ -1383,15 +1383,6 @@ static int onenand_read_oob_nolock(struct > mtd_info *mtd, loff_t from, return -EINVAL; > } > > - /* Do not allow reads past end of device */ > - if (unlikely(from >= mtd->size || > - column + len > ((mtd->size >> this->page_shift) > - > - (from >> this->page_shift)) * > oobsize)) { > - printk(KERN_ERR "%s: Attempted to read beyond end of > device\n", > - __func__); > - return -EINVAL; > - } > - > stats = mtd->ecc_stats; > > readcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_READ : > ONENAND_CMD_READOOB; @@ -2024,15 +2015,6 @@ static int > onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to, return > -EINVAL; } > > - /* Do not allow reads past end of device */ > - if (unlikely(to >= mtd->size || > - column + len > ((mtd->size >> this->page_shift) > - > - (to >> this->page_shift)) * > oobsize)) { > - printk(KERN_ERR "%s: Attempted to write past end of > device\n", > - __func__); > - return -EINVAL; > - } > - > oobbuf = this->oob_buf; > > oobcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_PROG : > ONENAND_CMD_PROGOOB;
On Mon, 8 Jan 2018 23:04:55 +0100 Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > Hello Boris, > > On Mon, 8 Jan 2018 22:15:42 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > Some of the check done in custom ->_read/write_oob() implementation > > are already done by the core (in mtd_check_oob_ops()). > > Not sure this is relevant here as your series introduces changes for > the SPI NAND framework, but there are other places where these checks > are, IMHO, also redundant and could be removed. The "past end" string > when grepped in the MTD folder core returns a few more hits. > > In the NAND core: > - nand_do_read_oob() > - nand_read_oob() > - nand_do_write_oob() > - nand_write_oob() That's true for nand_read/write_oob(). The one in nand_do_write_oob() is still needed because mtd_check_oob_ops() allows OOB writes crossing a page boundary. Finally, I don't see any boundary checks in nand_do_read_oob(). > Maybe also in onenand_base.c, but I am less confident for this one: > - onenand_bbt_read_oob() Unfortunately no, this function is directly called from onenand_bbt.c, which means the MTD layer layer is completely bypassed. I also found boundary checks in onenand_mlc_read_ops_nolock(), but again, this function is called from do_otp_read() which is not going through mtd_check_oob_ops(). > > What do you think? I'll remove the extra checks in nand_read/write_oob(). For the other ones, one solution would be to expose mtd_check_oob_ops(), but I'll keep that for later.
Hello Boris, On Mon, 8 Jan 2018 23:30:10 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 8 Jan 2018 23:04:55 +0100 > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > Hello Boris, > > > > On Mon, 8 Jan 2018 22:15:42 +0100 > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > Some of the check done in custom ->_read/write_oob() > > > implementation are already done by the core (in > > > mtd_check_oob_ops()). > > > > Not sure this is relevant here as your series introduces changes for > > the SPI NAND framework, but there are other places where these > > checks are, IMHO, also redundant and could be removed. The "past > > end" string when grepped in the MTD folder core returns a few more > > hits. > > > > In the NAND core: > > - nand_do_read_oob() > > - nand_read_oob() > > - nand_do_write_oob() > > - nand_write_oob() > > That's true for nand_read/write_oob(). The one in nand_do_write_oob() > is still needed because mtd_check_oob_ops() allows OOB writes > crossing a page boundary. Finally, I don't see any boundary checks in > nand_do_read_oob(). I forgot that crossing page boundaries was not a use case of mtd_check_oob_ops(), thanks for pointing it. However in nand_do_read/write_oob(), the comment and the code really state the checked boundary is the end of the device. So are you sure these two checks are needed? [1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2226 [2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2886 > > > Maybe also in onenand_base.c, but I am less confident for this one: > > - onenand_bbt_read_oob() > > Unfortunately no, this function is directly called from onenand_bbt.c, > which means the MTD layer layer is completely bypassed. I also found > boundary checks in onenand_mlc_read_ops_nolock(), but again, this > function is called from do_otp_read() which is not going through > mtd_check_oob_ops(). > > > > > What do you think? > > I'll remove the extra checks in nand_read/write_oob(). For the other > ones, one solution would be to expose mtd_check_oob_ops(), but I'll > keep that for later. Ok. Thanks, Miquèl
On Tue, 9 Jan 2018 08:19:53 +0100 Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > Hello Boris, > > On Mon, 8 Jan 2018 23:30:10 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > On Mon, 8 Jan 2018 23:04:55 +0100 > > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > > > Hello Boris, > > > > > > On Mon, 8 Jan 2018 22:15:42 +0100 > > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > > > Some of the check done in custom ->_read/write_oob() > > > > implementation are already done by the core (in > > > > mtd_check_oob_ops()). > > > > > > Not sure this is relevant here as your series introduces changes for > > > the SPI NAND framework, but there are other places where these > > > checks are, IMHO, also redundant and could be removed. The "past > > > end" string when grepped in the MTD folder core returns a few more > > > hits. > > > > > > In the NAND core: > > > - nand_do_read_oob() > > > - nand_read_oob() > > > - nand_do_write_oob() > > > - nand_write_oob() > > > > That's true for nand_read/write_oob(). The one in nand_do_write_oob() > > is still needed because mtd_check_oob_ops() allows OOB writes > > crossing a page boundary. Finally, I don't see any boundary checks in > > nand_do_read_oob(). > > I forgot that crossing page boundaries was not a use case of > mtd_check_oob_ops(), thanks for pointing it. However in > nand_do_read/write_oob(), the comment and the code really state the > checked boundary is the end of the device. So are you sure these two > checks are needed? > > [1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2226 > [2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2886 You mean, the lines I remove in this patch? :P > > > > > > Maybe also in onenand_base.c, but I am less confident for this one: > > > - onenand_bbt_read_oob() > > > > Unfortunately no, this function is directly called from onenand_bbt.c, > > which means the MTD layer layer is completely bypassed. I also found > > boundary checks in onenand_mlc_read_ops_nolock(), but again, this > > function is called from do_otp_read() which is not going through > > mtd_check_oob_ops(). > > > > > > > > What do you think? > > > > I'll remove the extra checks in nand_read/write_oob(). For the other > > ones, one solution would be to expose mtd_check_oob_ops(), but I'll > > keep that for later. > > Ok. > > Thanks, > Miquèl
On Tue, 9 Jan 2018 09:11:21 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Tue, 9 Jan 2018 08:19:53 +0100 > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > Hello Boris, > > > > On Mon, 8 Jan 2018 23:30:10 +0100 > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > On Mon, 8 Jan 2018 23:04:55 +0100 > > > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > > > > > Hello Boris, > > > > > > > > On Mon, 8 Jan 2018 22:15:42 +0100 > > > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > > > > > Some of the check done in custom ->_read/write_oob() > > > > > implementation are already done by the core (in > > > > > mtd_check_oob_ops()). > > > > > > > > Not sure this is relevant here as your series introduces > > > > changes for the SPI NAND framework, but there are other places > > > > where these checks are, IMHO, also redundant and could be > > > > removed. The "past end" string when grepped in the MTD folder > > > > core returns a few more hits. > > > > > > > > In the NAND core: > > > > - nand_do_read_oob() > > > > - nand_read_oob() > > > > - nand_do_write_oob() > > > > - nand_write_oob() > > > > > > That's true for nand_read/write_oob(). The one in > > > nand_do_write_oob() is still needed because mtd_check_oob_ops() > > > allows OOB writes crossing a page boundary. Finally, I don't see > > > any boundary checks in nand_do_read_oob(). > > > > I forgot that crossing page boundaries was not a use case of > > mtd_check_oob_ops(), thanks for pointing it. However in > > nand_do_read/write_oob(), the comment and the code really state the > > checked boundary is the end of the device. So are you sure these two > > checks are needed? > > > > [1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2226 > > [2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2886 > > You mean, the lines I remove in this patch? :P Oops, sorry for the noise then! Thanks, Miquèl
diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c index 5fb5e93d1547..a85af236b44d 100644 --- a/drivers/mtd/devices/docg3.c +++ b/drivers/mtd/devices/docg3.c @@ -904,9 +904,6 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from, if (ooblen % DOC_LAYOUT_OOB_SIZE) return -EINVAL; - if (from + len > mtd->size) - return -EINVAL; - ops->oobretlen = 0; ops->retlen = 0; ret = 0; @@ -1441,8 +1438,6 @@ static int doc_write_oob(struct mtd_info *mtd, loff_t ofs, if (len && ooblen && (len / DOC_LAYOUT_PAGE_SIZE) != (ooblen / oobdelta)) return -EINVAL; - if (ofs + len > mtd->size) - return -EINVAL; ops->oobretlen = 0; ops->retlen = 0; diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 07ef168ded98..131bb1d52871 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -108,25 +108,6 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_ecc_stats stats; int res; - if (from >= mtd->size) - return -EINVAL; - if (ops->datbuf && from + ops->len > mtd->size) - return -EINVAL; - - /* - * If OOB is also requested, make sure that we do not read past the end - * of this partition. - */ - if (ops->oobbuf) { - size_t len, pages; - - len = mtd_oobavail(mtd, ops); - pages = mtd_div_by_ws(mtd->size, mtd); - pages -= mtd_div_by_ws(from, mtd); - if (ops->ooboffs + ops->ooblen > pages * len) - return -EINVAL; - } - res = part->parent->_read_oob(part->parent, from + part->offset, ops); if (unlikely(mtd_is_eccerr(res))) mtd->ecc_stats.failed += @@ -190,10 +171,6 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to, { struct mtd_part *part = mtd_to_part(mtd); - if (to >= mtd->size) - return -EINVAL; - if (ops->datbuf && to + ops->len > mtd->size) - return -EINVAL; return part->parent->_write_oob(part->parent, to + part->offset, ops); } diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 889ceadbf607..61eeac233683 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2187,21 +2187,6 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, len = mtd_oobavail(mtd, ops); - if (unlikely(ops->ooboffs >= len)) { - pr_debug("%s: attempt to start read outside oob\n", - __func__); - return -EINVAL; - } - - /* Do not allow reads past end of device */ - if (unlikely(from >= mtd->size || - ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) - - (from >> chip->page_shift)) * len)) { - pr_debug("%s: attempt to read beyond end of device\n", - __func__); - return -EINVAL; - } - chipnr = (int)(from >> chip->chip_shift); chip->select_chip(mtd, chipnr); @@ -2820,22 +2805,6 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to, return -EINVAL; } - if (unlikely(ops->ooboffs >= len)) { - pr_debug("%s: attempt to start write outside oob\n", - __func__); - return -EINVAL; - } - - /* Do not allow write past end of device */ - if (unlikely(to >= mtd->size || - ops->ooboffs + ops->ooblen > - ((mtd->size >> chip->page_shift) - - (to >> chip->page_shift)) * len)) { - pr_debug("%s: attempt to write beyond end of device\n", - __func__); - return -EINVAL; - } - chipnr = (int)(to >> chip->chip_shift); /* diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 050ba8a87543..979f4031f23c 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -1383,15 +1383,6 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from, return -EINVAL; } - /* Do not allow reads past end of device */ - if (unlikely(from >= mtd->size || - column + len > ((mtd->size >> this->page_shift) - - (from >> this->page_shift)) * oobsize)) { - printk(KERN_ERR "%s: Attempted to read beyond end of device\n", - __func__); - return -EINVAL; - } - stats = mtd->ecc_stats; readcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_READ : ONENAND_CMD_READOOB; @@ -2024,15 +2015,6 @@ static int onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to, return -EINVAL; } - /* Do not allow reads past end of device */ - if (unlikely(to >= mtd->size || - column + len > ((mtd->size >> this->page_shift) - - (to >> this->page_shift)) * oobsize)) { - printk(KERN_ERR "%s: Attempted to write past end of device\n", - __func__); - return -EINVAL; - } - oobbuf = this->oob_buf; oobcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_PROG : ONENAND_CMD_PROGOOB;
Some of the check done in custom ->_read/write_oob() implementation are already done by the core (in mtd_check_oob_ops()). Suggested-by: Peter Pan <peterpansjtu@gmail.com> [Remove redundant checks done in mtdpart.c] Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- Changes in v4: - integrated into the SPI NAND preparation patchset Changes in v3: - Rebased on mtd/next after "mtd: Stop directly calling master ->_xxx() hooks from mtdpart code" had been dropped Changes in v2: - Merge "mtd: Remove unneeded checks in part_{read,write}_oob()" into this commit --- drivers/mtd/devices/docg3.c | 5 ----- drivers/mtd/mtdpart.c | 23 ----------------------- drivers/mtd/nand/nand_base.c | 31 ------------------------------- drivers/mtd/onenand/onenand_base.c | 18 ------------------ 4 files changed, 77 deletions(-)