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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9e3e0739-b859-4a62-954e-2b13f7d5dd85@amperemail.onmicrosoft.com>
Date: Mon, 2 Jun 2025 16:51:29 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: "lihuisong (C)" <lihuisong@...wei.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Sudeep Holla <sudeep.holla@....com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 admiyo@...amperecomputing.com, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>,
 Jeremy Kerr <jk@...econstruct.com.au>, Eric Dumazet <edumazet@...gle.com>,
 Matt Johnston <matt@...econstruct.com.au>, Paolo Abeni <pabeni@...hat.com>,
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v20 1/1] mctp pcc: Implement MCTP over PCC
 Transport


On 5/30/25 02:19, lihuisong (C) wrote:
>
> 在 2025/4/29 2:48, Adam Young 写道:
>>
>> On 4/24/25 09:03, lihuisong (C) wrote:
>>>> +    rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
>>>> +                     context.inbox_index);
>>>> +    if (rc)
>>>> +        goto free_netdev;
>>>> +    mctp_pcc_ndev->inbox.client.rx_callback = 
>>>> mctp_pcc_client_rx_callback;
>>> It is good to move the assignemnt of  rx_callback pointer to 
>>> initialize inbox mailbox.
>>
>>
>> The other changes are fine, but this one I do not agree with.
>>
>> The rx callback only makes sense for one of the two mailboxes, and 
>> thus is not appropriate for a generic function.
>>
>> Either  initialize_mailbox needs more complex logic, or would blindly 
>> assign the callback to both mailboxes, neither of which simplifies or 
>> streamlines the code.  That function emerged as a way to reduce 
>> duplication.  Lets keep it that way.
>>
> It depends on you. But please reply my below comment. I didn't see any 
> change about it in next version.
>
> -->
>
>> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct 
>> net_device *ndev)
>> +{
>> +    struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
>> +    struct mctp_pcc_hdr *mctp_pcc_header;
>> +    void __iomem *buffer;
>> +    unsigned long flags;
>> +    int len = skb->len;
>> +    int rc;
>> +
>> +    rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
>> +    if (rc)
>> +        goto err_drop;
>> +
>> +    mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
>> +    mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | 
>> mpnd->outbox.index);
>> +    mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
>> +    memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
>> +           MCTP_SIGNATURE_LENGTH);
>> +    mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
>> +
>> +    spin_lock_irqsave(&mpnd->lock, flags);
>> +    buffer = mpnd->outbox.chan->shmem;
>> +    memcpy_toio(buffer, skb->data, skb->len);
>> + 
>> mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
>> +                            NULL);
>> +    spin_unlock_irqrestore(&mpnd->lock, flags);
>> +
> Why does it not need to know if the packet is sent successfully?
> It's possible for the platform not to finish to send the packet after 
> executing this unlock.
> In this moment, the previous packet may be modified by the new packet 
> to be sent.

I think you missed version  21.

Version 21 of this function ends with:
         memcpy_toio(buffer, skb->data, skb->len);
         rc = mpnd->outbox.chan->mchan->mbox->ops->send_data
                 (mpnd->outbox.chan->mchan, NULL);
         spin_unlock_irqrestore(&mpnd->lock, flags);
         if ACPI_FAILURE(rc)
                 goto err_drop;
         dev_dstats_tx_add(ndev, len);
         dev_consume_skb_any(skb);
         return NETDEV_TX_OK;
err_drop:
         dev_dstats_tx_dropped(ndev);
         kfree_skb(skb);
         return NETDEV_TX_OK;


Once the memcpy_toio completes, the driver will not look at the packet 
again.  if the Kernel did change it at this point, it would not affect 
the flow.  The send of the packet is checked vi rc returned from 
send_data, and it tags the packet as dropped.  Is this not sufficient?





Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ