From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Peter Pan <peterpansjtu@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
Marek Vasut <marek.vasut@gmail.com>,
Richard Weinberger <richard@nod.at>,
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
linux-mtd@lists.infradead.org,
Robert Jarzmik <robert.jarzmik@free.fr>,
Kyungmin Park <kyungmin.park@samsung.com>,
Frieder Schrempf <frieder.schrempf@exceet.de>
Subject: Re: [PATCH v3 3/4] mtd: Stop directly calling master ->_xxx() hooks from mtdpart code
Date: Fri, 22 Dec 2017 09:37:50 +0100 [thread overview]
Message-ID: <20171222093750.0c49abec@bbrezillon> (raw)
In-Reply-To: <CAAyFORLBhxpn5cFHztrY5OHt9FCfbDkQzUCsLf2Wyp2aGzPGnA@mail.gmail.com>
On Fri, 22 Dec 2017 13:40:26 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:
> Hi Boris,
>
> On Fri, Dec 15, 2017 at 8:39 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > The MTD layer provides several wrappers around mtd->_xxx() hooks. Call
> > these wrappers instead of directly dereferencing the associated ->_xxx()
> > pointer.
> >
> > This change has been motivated by another rework letting the core
> > handle the case where ->_read/write_oob() are implemented but not
> > ->_read/write(). In this case, we want mtd_read/write() to fall back to
> > ->_read/write_oob() when ->_read/write() are NULL. The problem is,
> > mtdpart is directly calling the ->_xxx() instead of using the wrappers,
> > thus leading to a NULL pointer exception.
> >
> > Even though we only need to do the change for part_read/write(), going
> > through those wrappers for all kind of part -> master operation
> > propagation is a good thing, because other wrappers might become
> > smarter over time, and the duplicated check overhead (parameters will
> > be checked at the partition and master level instead of only at the
> > partition level) should be negligible.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > Changes in v3:
> > - unconditionally assign part wrappers as suggested by Brian
> >
> > Changes in v2:
> > - new patch needed to fix a NULL pointer dereference BUG
> > ---
> > drivers/mtd/mtdpart.c | 141 +++++++++++++++++++-------------------------------
> > 1 file changed, 53 insertions(+), 88 deletions(-)
> >
> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> > index be088bccd593..e83c9d870b11 100644
> > --- a/drivers/mtd/mtdpart.c
> > +++ b/drivers/mtd/mtdpart.c
> > @@ -74,8 +74,7 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
> > int res;
> >
>
> This is not about your modification. But shouldn't we add check to prevent
> part_read/write/write_oob from accessing past the end of partition?
> There is a check in part_read_oob() only.
You should not call part_xxx() directly, and mtd_read/write{_oob}()
should already check that. If that's not the case, we should fix them.
Can you give a bit more details about what is wrong?
Thanks,
Boris
>
> Thanks
> Peter Pan
>
> > stats = part->parent->ecc_stats;
> > - res = part->parent->_read(part->parent, from + part->offset, len,
> > - retlen, buf);
> > + res = mtd_read(part->parent, from + part->offset, len, retlen, buf);
> > if (unlikely(mtd_is_eccerr(res)))
> > mtd->ecc_stats.failed +=
> > part->parent->ecc_stats.failed - stats.failed;
> > @@ -90,15 +89,15 @@ static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> >
> > - return part->parent->_point(part->parent, from + part->offset, len,
> > - retlen, virt, phys);
> > + return mtd_point(part->parent, from + part->offset, len, retlen, virt,
> > + phys);
> > }
> >
> > static int part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> >
> > - return part->parent->_unpoint(part->parent, from + part->offset, len);
> > + return mtd_unpoint(part->parent, from + part->offset, len);
> > }
> >
> > static int part_read_oob(struct mtd_info *mtd, loff_t from,
> > @@ -126,7 +125,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
> > return -EINVAL;
> > }
> >
> > - res = part->parent->_read_oob(part->parent, from + part->offset, ops);
> > + res = mtd_read_oob(part->parent, from + part->offset, ops);
> > if (unlikely(res)) {
> > if (mtd_is_bitflip(res))
> > mtd->ecc_stats.corrected++;
> > @@ -140,48 +139,43 @@ static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
> > size_t len, size_t *retlen, u_char *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_read_user_prot_reg(part->parent, from, len,
> > - retlen, buf);
> > + return mtd_read_user_prot_reg(part->parent, from, len, retlen, buf);
> > }
> >
> > static int part_get_user_prot_info(struct mtd_info *mtd, size_t len,
> > size_t *retlen, struct otp_info *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_get_user_prot_info(part->parent, len, retlen,
> > - buf);
> > + return mtd_get_user_prot_info(part->parent, len, retlen, buf);
> > }
> >
> > static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
> > size_t len, size_t *retlen, u_char *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_read_fact_prot_reg(part->parent, from, len,
> > - retlen, buf);
> > + return mtd_read_fact_prot_reg(part->parent, from, len, retlen, buf);
> > }
> >
> > static int part_get_fact_prot_info(struct mtd_info *mtd, size_t len,
> > size_t *retlen, struct otp_info *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_get_fact_prot_info(part->parent, len, retlen,
> > - buf);
> > + return mtd_get_fact_prot_info(part->parent, len, retlen, buf);
> > }
> >
> > static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
> > size_t *retlen, const u_char *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_write(part->parent, to + part->offset, len,
> > - retlen, buf);
> > + return mtd_write(part->parent, to + part->offset, len, retlen, buf);
> > }
> >
> > static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
> > size_t *retlen, const u_char *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_panic_write(part->parent, to + part->offset, len,
> > - retlen, buf);
> > + return mtd_panic_write(part->parent, to + part->offset, len, retlen,
> > + buf);
> > }
> >
> > static int part_write_oob(struct mtd_info *mtd, loff_t to,
> > @@ -193,30 +187,29 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to,
> > return -EINVAL;
> > if (ops->datbuf && to + ops->len > mtd->size)
> > return -EINVAL;
> > - return part->parent->_write_oob(part->parent, to + part->offset, ops);
> > + return mtd_write_oob(part->parent, to + part->offset, ops);
> > }
> >
> > static int part_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
> > size_t len, size_t *retlen, u_char *buf)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_write_user_prot_reg(part->parent, from, len,
> > - retlen, buf);
> > + return mtd_write_user_prot_reg(part->parent, from, len, retlen, buf);
> > }
> >
> > static int part_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
> > size_t len)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_lock_user_prot_reg(part->parent, from, len);
> > + return mtd_lock_user_prot_reg(part->parent, from, len);
> > }
> >
> > static int part_writev(struct mtd_info *mtd, const struct kvec *vecs,
> > unsigned long count, loff_t to, size_t *retlen)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_writev(part->parent, vecs, count,
> > - to + part->offset, retlen);
> > + return mtd_writev(part->parent, vecs, count, to + part->offset,
> > + retlen);
> > }
> >
> > static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
> > @@ -225,7 +218,7 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
> > int ret;
> >
> > instr->addr += part->offset;
> > - ret = part->parent->_erase(part->parent, instr);
> > + ret = mtd_erase(part->parent, instr);
> > if (ret) {
> > if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
> > instr->fail_addr -= part->offset;
> > @@ -251,51 +244,51 @@ EXPORT_SYMBOL_GPL(mtd_erase_callback);
> > static int part_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_lock(part->parent, ofs + part->offset, len);
> > + return mtd_lock(part->parent, ofs + part->offset, len);
> > }
> >
> > static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_unlock(part->parent, ofs + part->offset, len);
> > + return mtd_unlock(part->parent, ofs + part->offset, len);
> > }
> >
> > static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_is_locked(part->parent, ofs + part->offset, len);
> > + return mtd_is_locked(part->parent, ofs + part->offset, len);
> > }
> >
> > static void part_sync(struct mtd_info *mtd)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - part->parent->_sync(part->parent);
> > + mtd_sync(part->parent);
> > }
> >
> > static int part_suspend(struct mtd_info *mtd)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_suspend(part->parent);
> > + return mtd_suspend(part->parent);
> > }
> >
> > static void part_resume(struct mtd_info *mtd)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - part->parent->_resume(part->parent);
> > + mtd_resume(part->parent);
> > }
> >
> > static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > ofs += part->offset;
> > - return part->parent->_block_isreserved(part->parent, ofs);
> > + return mtd_block_isreserved(part->parent, ofs);
> > }
> >
> > static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > ofs += part->offset;
> > - return part->parent->_block_isbad(part->parent, ofs);
> > + return mtd_block_isbad(part->parent, ofs);
> > }
> >
> > static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
> > @@ -304,7 +297,7 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
> > int res;
> >
> > ofs += part->offset;
> > - res = part->parent->_block_markbad(part->parent, ofs);
> > + res = mtd_block_markbad(part->parent, ofs);
> > if (!res)
> > mtd->ecc_stats.badblocks++;
> > return res;
> > @@ -313,13 +306,13 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
> > static int part_get_device(struct mtd_info *mtd)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - return part->parent->_get_device(part->parent);
> > + return __get_mtd_device(part->parent);
> > }
> >
> > static void part_put_device(struct mtd_info *mtd)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> > - part->parent->_put_device(part->parent);
> > + __put_mtd_device(part->parent);
> > }
> >
> > static int part_ooblayout_ecc(struct mtd_info *mtd, int section,
> > @@ -347,8 +340,7 @@ static int part_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
> > {
> > struct mtd_part *part = mtd_to_part(mtd);
> >
> > - return part->parent->_max_bad_blocks(part->parent,
> > - ofs + part->offset, len);
> > + return mtd_max_bad_blocks(part->parent, ofs + part->offset, len);
> > }
> >
> > static inline void free_partition(struct mtd_part *p)
> > @@ -437,59 +429,32 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent,
> >
> > slave->mtd._read = part_read;
> > slave->mtd._write = part_write;
> > -
> > - if (parent->_panic_write)
> > - slave->mtd._panic_write = part_panic_write;
> > -
> > - if (parent->_point && parent->_unpoint) {
> > - slave->mtd._point = part_point;
> > - slave->mtd._unpoint = part_unpoint;
> > - }
> > -
> > - if (parent->_read_oob)
> > - slave->mtd._read_oob = part_read_oob;
> > - if (parent->_write_oob)
> > - slave->mtd._write_oob = part_write_oob;
> > - if (parent->_read_user_prot_reg)
> > - slave->mtd._read_user_prot_reg = part_read_user_prot_reg;
> > - if (parent->_read_fact_prot_reg)
> > - slave->mtd._read_fact_prot_reg = part_read_fact_prot_reg;
> > - if (parent->_write_user_prot_reg)
> > - slave->mtd._write_user_prot_reg = part_write_user_prot_reg;
> > - if (parent->_lock_user_prot_reg)
> > - slave->mtd._lock_user_prot_reg = part_lock_user_prot_reg;
> > - if (parent->_get_user_prot_info)
> > - slave->mtd._get_user_prot_info = part_get_user_prot_info;
> > - if (parent->_get_fact_prot_info)
> > - slave->mtd._get_fact_prot_info = part_get_fact_prot_info;
> > - if (parent->_sync)
> > - slave->mtd._sync = part_sync;
> > - if (!partno && !parent->dev.class && parent->_suspend &&
> > - parent->_resume) {
> > + slave->mtd._panic_write = part_panic_write;
> > + slave->mtd._point = part_point;
> > + slave->mtd._unpoint = part_unpoint;
> > + slave->mtd._read_oob = part_read_oob;
> > + slave->mtd._write_oob = part_write_oob;
> > + slave->mtd._read_user_prot_reg = part_read_user_prot_reg;
> > + slave->mtd._read_fact_prot_reg = part_read_fact_prot_reg;
> > + slave->mtd._write_user_prot_reg = part_write_user_prot_reg;
> > + slave->mtd._lock_user_prot_reg = part_lock_user_prot_reg;
> > + slave->mtd._get_user_prot_info = part_get_user_prot_info;
> > + slave->mtd._get_fact_prot_info = part_get_fact_prot_info;
> > + slave->mtd._sync = part_sync;
> > + if (!partno && !parent->dev.class) {
> > slave->mtd._suspend = part_suspend;
> > slave->mtd._resume = part_resume;
> > }
> > - if (parent->_writev)
> > - slave->mtd._writev = part_writev;
> > - if (parent->_lock)
> > - slave->mtd._lock = part_lock;
> > - if (parent->_unlock)
> > - slave->mtd._unlock = part_unlock;
> > - if (parent->_is_locked)
> > - slave->mtd._is_locked = part_is_locked;
> > - if (parent->_block_isreserved)
> > - slave->mtd._block_isreserved = part_block_isreserved;
> > - if (parent->_block_isbad)
> > - slave->mtd._block_isbad = part_block_isbad;
> > - if (parent->_block_markbad)
> > - slave->mtd._block_markbad = part_block_markbad;
> > - if (parent->_max_bad_blocks)
> > - slave->mtd._max_bad_blocks = part_max_bad_blocks;
> > -
> > - if (parent->_get_device)
> > - slave->mtd._get_device = part_get_device;
> > - if (parent->_put_device)
> > - slave->mtd._put_device = part_put_device;
> > + slave->mtd._writev = part_writev;
> > + slave->mtd._lock = part_lock;
> > + slave->mtd._unlock = part_unlock;
> > + slave->mtd._is_locked = part_is_locked;
> > + slave->mtd._block_isreserved = part_block_isreserved;
> > + slave->mtd._block_isbad = part_block_isbad;
> > + slave->mtd._block_markbad = part_block_markbad;
> > + slave->mtd._max_bad_blocks = part_max_bad_blocks;
> > + slave->mtd._get_device = part_get_device;
> > + slave->mtd._put_device = part_put_device;
> >
> > slave->mtd._erase = part_erase;
> > slave->parent = parent;
> > --
> > 2.11.0
> >
next prev parent reply other threads:[~2017-12-22 8:38 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-15 12:39 [PATCH v3 0/4] mtd: Preparation patches for the SPI NAND framework Boris Brezillon
2017-12-15 12:39 ` [PATCH v3 1/4] mtd: Do not allow MTD devices with inconsistent erase properties Boris Brezillon
2017-12-15 12:39 ` [PATCH v3 2/4] mtd: Add an helper to make erase request aligned on ->erasesize Boris Brezillon
2017-12-15 12:39 ` [PATCH v3 3/4] mtd: Stop directly calling master ->_xxx() hooks from mtdpart code Boris Brezillon
2017-12-22 5:40 ` Peter Pan
2017-12-22 8:37 ` Boris Brezillon [this message]
2018-01-04 2:06 ` Peter Pan
2018-01-08 20:46 ` Boris Brezillon
2017-12-15 12:39 ` [PATCH v3 4/4] mtd: Fallback to ->_read/write_oob() when ->_read/write() is missing Boris Brezillon
2018-01-08 21:11 ` Boris Brezillon
2017-12-17 21:39 ` [PATCH v3 0/4] mtd: Preparation patches for the SPI NAND framework Miquel RAYNAL
2018-01-06 20:43 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171222093750.0c49abec@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=computersforpeace@gmail.com \
--cc=cyrille.pitchen@wedev4u.fr \
--cc=dwmw2@infradead.org \
--cc=frieder.schrempf@exceet.de \
--cc=kyungmin.park@samsung.com \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=peterpansjtu@gmail.com \
--cc=richard@nod.at \
--cc=robert.jarzmik@free.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.