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@xxxxxxxxxxxxxxxxxxx> 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
