On 6 September 2016 at 15:19, Andreas Mohr <andi@xxxxxxxx> wrote: > On Tue, Sep 06, 2016 at 02:26:06PM +0800, Baolin Wang wrote: >> On 6 September 2016 at 12:34, Andreas Mohr <andi@xxxxxxxx> wrote: >> >> - to = from + nr; >> >> - >> >> - if (to <= from) >> >> - return -EINVAL; >> > >> > Hmm, this is swallowing -EINVAL behaviour >> > i.e., now possibly violating protocol? >> >> I didn't see what situation will make variable 'to' is less than >> 'from' since I think variable 'nr' is always larger than 0, right? If >> so, we should remove this useless checking. Thanks. > > Hmm, indeed, since all participating variables are unsigned, > the existing calculation should never hit this. > However, one could argue that this is an additional safeguard > against implementation source getting modified in a way > that will suddenly result in this pathologic case becoming true > (where a -EINVAL bailout surely will then pinpoint things > much more visibly for some users, > as opposed to potential data corruption or some such). OK. I'll add this checking. > > > > I have seen another change > >> - if (nr == 0) >> - return 0; > > where it gets moved out of common path > and into MMC_ERASE_ARG-specific branch > (likely because the subsequent common-path conditional of > nr > rem > is deemed sufficient). > > This seems to be again a change > where a simple yet crucial > device geometry calculation post-condition > (either to > from, or nr > 0) > is then not verified specifically/separately. > > Ultimately, I am not sure whether or not > these (post-)conditions should be verified > in their most basic, simple form, > as an extra/separate verification step. After some investigation, I think we add this checking is more safer. Thanks for your comments. -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
