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

From	Baolin Wang <baolin.wang@linaro.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size
Date	2016-09-06 10:30 +0200
Message-ID	<sejVw-7Vd-29@gated-at.bofh.it> (permalink)
References	<seeM9-4kM-3@gated-at.bofh.it> <seeM9-4kM-1@gated-at.bofh.it> <segkV-5tu-17@gated-at.bofh.it> <sei3n-6DX-9@gated-at.bofh.it> <seiPM-7bK-13@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On 6 September 2016 at 15:19, Andreas Mohr <andi@lisas.de> wrote:
> On Tue, Sep 06, 2016 at 02:26:06PM +0800, Baolin Wang wrote:
>> On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> 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

Thread

[PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 05:00 +0200
  Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Andreas Mohr <andi@lisas.de> - 2016-09-06 06:40 +0200
    Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 08:30 +0200
      Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Andreas Mohr <andi@lisas.de> - 2016-09-06 09:20 +0200
        Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 10:30 +0200
      Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Adrian Hunter <adrian.hunter@intel.com> - 2016-09-06 10:00 +0200
        Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 10:30 +0200

csiph-web