Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[Baolin Wang]

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