netdev - Re: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 12:51:49 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Jeremy Kerr <jk@...econstruct.com.au>, admiyo@...amperecomputing.com,
 Matt Johnston <matt@...econstruct.com.au>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport

Yep, you are right, and as I continue to mull this over, I realize I can 
make it much simpler.

Sorry about missing the changelog entry, as I had written that prior to 
realizing that all of this cleanup was a workaround for the error you 
found in the first iteration:  flipping the null meant I was leaving 
behind devices when I should have been cleaning them up.  So, yeah, I 
think the list can go now.

On 6/28/24 04:41, Jeremy Kerr wrote:
> You can save the list_entry() below by using list_for_each_entry_safe()
> here.
>
>> +               struct net_device *ndev;
>> +               struct mctp_pcc_ndev *mctp_pcc_dev;
>> +
>> +               mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
>> +               if (mctp_pcc_dev->acpi_device != adev)
>> +                       continue;
> Wait, you removed the case where you clean up the whole list if adev is
> NULL?
>
> Now this contradicts your section of the changelog:
>
> - Did not change the module init unload function to use the
>    ACPI equivalent as they do not remove all devices prior
>    to unload, leading to dangling references and seg faults.
>
> Does this mean you no longer need to free all instances on unload? In
> which case, that sounds great! that probably also means:
>
>   - all the list is doing is allowing you to find the mctp_pcc_dev from
>     the acpi_device
>
>   - but: you can turn an acpi_device into a mctp_pcc_dev from
>     adev->driver_data (which you're already setting), so you don't need
>     the list
>
>   - then, _remove just becomes something like:
>
>      static void mctp_pcc_driver_remove(struct acpi_device *adev)
>      {
>              struct mctp_pcc_dev *dev = acpi_driver_data(adev);
>
>              pcc_mbox_free_channel(dev->out_chan);
>              pcc_mbox_free_channel(dev->in_chan);
>              if (dev->mdev.dev)
>                      mctp_unregister_netdev(dev->mdev.dev);
>      }
>
>      (but I can't see how dev->mdev.dev can be null, so you may be able
>      to remove the condition too)