lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Aug 2021 19:35:02 +0800 From: Zhihao Cheng <chengzhihao1@...wei.com> To: Miquel Raynal <miquel.raynal@...tlin.com> CC: <richard@....at>, <vigneshr@...com>, <bbrezillon@...nel.org>, <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, <yukuai3@...wei.com> Subject: Re: [PATCH 1/2] mtd: mtdconcat: Judge callback function existence getting from master for each partition 在 2021/8/7 18:32, Miquel Raynal 写道: Hi Miquel, > Hi Zhihao, > > Zhihao Cheng <chengzhihao1@...wei.com> wrote on Sat, 7 Aug 2021 > 10:15:46 +0800: > >> 在 2021/8/7 3:28, Miquel Raynal 写道: >> Hi Miquel, >>> Hi Zhihao, >>> >>> Zhihao Cheng <chengzhihao1@...wei.com> wrote on Sat, 31 Jul 2021 >>> 10:32:42 +0800: >>> @@ -721,14 +724,15 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[], /* subdevices to c >>> subdev[i]->flags & MTD_WRITEABLE; >>> } >>> > + subdev_master = mtd_get_master(subdev[i]); >>> concat->mtd.size += subdev[i]->size; >>> concat->mtd.ecc_stats.badblocks += >>> subdev[i]->ecc_stats.badblocks; >>> if (concat->mtd.writesize != subdev[i]->writesize || >>> concat->mtd.subpage_sft != subdev[i]->subpage_sft || >>> concat->mtd.oobsize != subdev[i]->oobsize || >>> - !concat->mtd._read_oob != !subdev[i]->_read_oob || >>> - !concat->mtd._write_oob != !subdev[i]->_write_oob) { >>> + !concat->mtd._read_oob != !subdev_master->_read_oob || >>> + !concat->mtd._write_oob != !subdev_master->_write_oob) { >>> Do you really need this change? >> I think both "!concat->mtd._read_oob != !subdev[i]->_read_oob" and "!concat->mtd._write_oob != !subdev[i]->_write_oob" need to be modified otherwise concatenating goes failure. >> >> I thought there exists two problems: >> >> 1. Wrong callback fetching in mtd partition device >> >> 2. Warning for existence of _read and _read_oob at the same time >> >> so I solved them in two steps to make history commit logs a bit clear. >> >> Though these two patches can be combined to one. > No please keep the split. > > What I mean here is that I don't think your fix is valid. Maybe we > should propagate these callbacks as well instead of trying to hack into > this condition. I don't see why you should check against subdev[i] for > half of the callbacks and check for subdev_master for the last two. I have the following understanding of mtd: 1. The existence of mtd partition device's callbacks(what can mtd do, _read, _write, _read_oob, etc.) is decided by mtd master device. All mtd common functions (mtd_read, mtd_read_oob) pass master mtd device rather than partition mtd device as parameter, because nand_chip(initialized as master mtd device) process requests. So there are two steps in mtd common function: fetch master mtd device; invoke master mtd devices's callback and pass in master mtd device. Propogating callbacks to partition mtd device may bring some imperfections: A. No adaptions to mtd common functions: partition mtd device's callbacks will never be invoked, they are only used to judge whether assigin concatenated device's callback while concatenating. Looks weird. @@ -86,6 +86,61 @@ static struct mtd_info *allocate_partition(struct mtd_info *parent, child->part.offset = part->offset; INIT_LIST_HEAD(&child->partitions); + if (parent->_read) + child->_read = parent->_read; + if (parent->_write) + child->_write = parent->_write; [...] + if (parent->_read_oob) + child->_read_oob = parent->_read_oob; + if (parent->_write_oob) B. Re-import removed partition mtd device's callbacks and adapt mtd common functions: Current implemention is simplier than the version before 46b5889cc2c54("mtd: implement proper partition handling"). Adapting mtd common functions is a risky thing, which could effect other modules, should we do that? +static int part_read(struct mtd_info *mtd, loff_t from, size_t len, + size_t *retlen, u_char *buf) +{ + struct mtd_part *part = mtd_to_part(mtd); + struct mtd_ecc_stats stats; + int res; + + stats = part->parent->ecc_stats; + res = part->parent->_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; + else + mtd->ecc_stats.corrected += + part->parent->ecc_stats.corrected - stats.corrected; + return res; +} static int mtd_read_oob_std(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) { - struct mtd_info *master = mtd_get_master(mtd); int ret; - from = mtd_get_master_ofs(mtd, from); - if (master->_read_oob) - ret = master->_read_oob(master, from, ops); + if (mtd->_read_oob) + ret = mtd->_read_oob(mtd, from, ops); else - ret = master->_read(master, from, ops->len, &ops->retlen, + ret = mtd->_read(mtd, from, ops->len, &ops->retlen, ops->datbuf); 2. Checking against subdev[i] for data members and check against subdev_master for the callbacks looks weird. I modified it based on the assumption that partition mtd device' callbacks inherit from master mtd device but data members(name, size) may not. Maybe I should add some comment to explain why checking against subdev[i] for data members and checking against subdev_master for the callbacks. So, which method is better? Any other method?
Powered by blists - more mailing lists