[GIT PULL] Revert ata fix for 5.17-rc2

[GIT PULL] Revert ata fix for 5.17-rc2

[Damien Le Moal]

Linus,

I forgot about the umn.edu situation and accepted a patch, which was the single
fix in my earlier pull request. This pull request reverts the patch. My
apologies for the noise.

The following changes since commit 9b6d90e2085ca2ce72ef9ea78658bf270855e62e:

  ata: pata_platform: Fix a NULL pointer dereference in __pata_platform_probe() (2022-01-27 11:22:43 +0900)

are available in the Git repository at:

  ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata tags/ata-5.17-rc2-revert

for you to fetch changes up to c3c7882349b92155737a2299d616f586211bde72:

  Revert "ata: pata_platform: Fix a NULL pointer dereference in __pata_platform_probe()" (2022-01-28 20:58:38 +0900)

----------------------------------------------------------------
Revert ata fix for 5.17.0-rc2

Revert patch from umn.edu as this instituion is not allowed to
contribute to the Linux kernel until it has properly resolved its
development issues.

----------------------------------------------------------------
Damien Le Moal (1):
      Revert "ata: pata_platform: Fix a NULL pointer dereference in __pata_platform_probe()"

 drivers/ata/pata_platform.c | 2 --
 1 file changed, 2 deletions(-)

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[Linus Torvalds]

On Fri, Jan 28, 2022 at 2:10 PM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> I forgot about the umn.edu situation and accepted a patch, which was the single
> fix in my earlier pull request. This pull request reverts the patch. My
> apologies for the noise.

Gaah. The patch looks valid, so I think reverting it just for the umn
situation is not only noise, but a bit unnecessary.

I'm adding Greg to the cc, since he was the one most involved with
that whole thing and maybe he feels otherwise, but I'll drop this
revert for now on the assumption that it's fine.

               Linus

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[James Bottomley]

On Fri, 2022-01-28 at 18:34 +0200, Linus Torvalds wrote:
> On Fri, Jan 28, 2022 at 2:10 PM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
> > I forgot about the umn.edu situation and accepted a patch, which
> > was the single
> > fix in my earlier pull request. This pull request reverts the
> > patch. My
> > apologies for the noise.
> 
> Gaah. The patch looks valid, so I think reverting it just for the umn
> situation is not only noise, but a bit unnecessary.
> 
> I'm adding Greg to the cc, since he was the one most involved with
> that whole thing and maybe he feels otherwise, but I'll drop this
> revert for now on the assumption that it's fine.

The patch as fixed by Damien looks reasonable, although I don't think
it actually fixes anything ... if we get an ENOMEM there the system
won't boot because you likely can't contact your root drive, so an oops
would make the point more vividly.

However, the original patch:

https://lore.kernel.org/all/20220124164525.53068-1-zhou1615@umn.edu/

does look suspiciously like a fishing attempt to see if we'd notice the
logic inversion in the supposed null check.

James

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[Linus Torvalds]

On Fri, Jan 28, 2022 at 6:57 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> However, the original patch:
>
> https://lore.kernel.org/all/20220124164525.53068-1-zhou1615@umn.edu/
>
> does look suspiciously like a fishing attempt to see if we'd notice the
> logic inversion in the supposed null check.

Right you are.

Gosh darn it, I thought the umn people already apologized for their
bad behavior, and yet here it is again.

It's good that I'm now a reformed person, because otherwise I might
use bad language.

                 Linus

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[Greg Kroah-Hartman]

On Fri, Jan 28, 2022 at 06:34:16PM +0200, Linus Torvalds wrote:
> On Fri, Jan 28, 2022 at 2:10 PM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
> >
> > I forgot about the umn.edu situation and accepted a patch, which was the single
> > fix in my earlier pull request. This pull request reverts the patch. My
> > apologies for the noise.
> 
> Gaah. The patch looks valid, so I think reverting it just for the umn
> situation is not only noise, but a bit unnecessary.
> 
> I'm adding Greg to the cc, since he was the one most involved with
> that whole thing and maybe he feels otherwise, but I'll drop this
> revert for now on the assumption that it's fine.

It's up to you all, if you think the patch is correct, keep it for now.
I had review comments on it yesterday here:
	https://lore.kernel.org/all/YfQSdJgi4x5hN3Ee@kroah.com/

but it seems like it might be right.

I have contacted the university again, and they privately apologized and
have said they will stop contributing until the process we worked on is
in place.  Someone decided to ignore their administration's rules here
it seems :(

thanks,

greg k-h

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[Linus Torvalds]

On Sat, Jan 29, 2022 at 8:59 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> It's up to you all, if you think the patch is correct, keep it for now.

In the fixed form (ie with Damien's fix for the wrong test polarity),
it's certainly not wrong, and matches a lot of our standard patterns -
including our documentation in

    Documentation/driver-api/driver-model/design-patterns.rst

I did a quick visual grep, and all the cases of
devm_kzalloc(..GFP_KERNEL) I grepped for did indeed have that "if
(!..)" error handling pattern for the return value, including other
cases in the ATA subsystem.

That was very much a "quick visual grep" though, so no guarantees, and
I stopped looking after it was so obvious. IOW, it was just a

   git grep -1 devm_kzalloc.*GFP_KERNEL

and then looking at the output and saying "yup, they all seem to do
that allocation failure test".

At the same time it's certainly also true that a small devm_kzalloc()
using GFP_KERNEL should never actually fail, so the patch - while
looking very correct to me - almost certainly makes no difference in
practice.

So I think the only problem with the patch is the original (fixed)
bug, and the source it came from.

              Linus

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[Sasha Levin]

On Sat, Jan 29, 2022 at 09:07:14AM +0200, Linus Torvalds wrote:
>On Sat, Jan 29, 2022 at 8:59 AM Greg Kroah-Hartman
><gregkh@linuxfoundation.org> wrote:
>>
>> It's up to you all, if you think the patch is correct, keep it for now.
>
>In the fixed form (ie with Damien's fix for the wrong test polarity),
>it's certainly not wrong, and matches a lot of our standard patterns -
>including our documentation in
>
>    Documentation/driver-api/driver-model/design-patterns.rst
>
>I did a quick visual grep, and all the cases of
>devm_kzalloc(..GFP_KERNEL) I grepped for did indeed have that "if
>(!..)" error handling pattern for the return value, including other
>cases in the ATA subsystem.
>
>That was very much a "quick visual grep" though, so no guarantees, and
>I stopped looking after it was so obvious. IOW, it was just a
>
>   git grep -1 devm_kzalloc.*GFP_KERNEL
>
>and then looking at the output and saying "yup, they all seem to do
>that allocation failure test".

I think that in this case the issue isn't the correctness of the
devm_kzalloc() allocation test itself, but rather the context in which
it's made in:

         host = ata_host_alloc(dev, 1);
         if (!host)
                 return -ENOMEM;
         ap = host->ports[0];

         ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
         if (!ap->ops)
                 return -ENOMEM;

My reading of ata_host_alloc() is that it allocates a refcounted 'struct
ata_host', but due to how we handle the failure of the following
devm_kzalloc(), we will never invoke ata_host_release() because the ref
won't be dropped anywhere.

So yes, the patch looks correct in the context shown by the patch
itself, but once we look at the entire function I think it's incorrect
(or, at least, I would expect more reasoning beyond "the static checker
told us so" around why it might be correct in the patch message itself
if I'm wrong).

-- 
Thanks,
Sasha

Re: [GIT PULL] Revert ata fix for 5.17-rc2

[Damien Le Moal]

On 2022/01/30 23:28, Sasha Levin wrote:
> On Sat, Jan 29, 2022 at 09:07:14AM +0200, Linus Torvalds wrote:
>> On Sat, Jan 29, 2022 at 8:59 AM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> It's up to you all, if you think the patch is correct, keep it for now.
>>
>> In the fixed form (ie with Damien's fix for the wrong test polarity),
>> it's certainly not wrong, and matches a lot of our standard patterns -
>> including our documentation in
>>
>>    Documentation/driver-api/driver-model/design-patterns.rst
>>
>> I did a quick visual grep, and all the cases of
>> devm_kzalloc(..GFP_KERNEL) I grepped for did indeed have that "if
>> (!..)" error handling pattern for the return value, including other
>> cases in the ATA subsystem.
>>
>> That was very much a "quick visual grep" though, so no guarantees, and
>> I stopped looking after it was so obvious. IOW, it was just a
>>
>>   git grep -1 devm_kzalloc.*GFP_KERNEL
>>
>> and then looking at the output and saying "yup, they all seem to do
>> that allocation failure test".
> 
> I think that in this case the issue isn't the correctness of the
> devm_kzalloc() allocation test itself, but rather the context in which
> it's made in:
> 
>          host = ata_host_alloc(dev, 1);
>          if (!host)
>                  return -ENOMEM;
>          ap = host->ports[0];
> 
>          ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
>          if (!ap->ops)
>                  return -ENOMEM;
> 
> My reading of ata_host_alloc() is that it allocates a refcounted 'struct
> ata_host', but due to how we handle the failure of the following
> devm_kzalloc(), we will never invoke ata_host_release() because the ref
> won't be dropped anywhere.

As explained in my reply to Greg email, the path is:

ata_devres_release() -> ata_hot_put() -> ata_host_release()

But for that to happen, the last ref on the dev must be dropped.

> So yes, the patch looks correct in the context shown by the patch
> itself, but once we look at the entire function I think it's incorrect
> (or, at least, I would expect more reasoning beyond "the static checker
> told us so" around why it might be correct in the patch message itself
> if I'm wrong).

As long as the dev last ref is dropped if there is a probe error, I think there
are no issues here. If that is not the case and the dev is not dropped on probe
error, then there is a leak, but then that leak likely exist for most ata
drivers. I will spend some time checking all this.

-- 
Damien Le Moal
Western Digital Research