Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

["Nelson, Shannon" <shannon.nelson@amd.com>]

On 3/4/2025 9:24 AM, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 05:08:08PM +0800, Jonathan Cameron wrote:
>>> +   dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
>>
>> Perhaps a little noisy as I think userspace can trigger this easily.  dev_dbg()
>> might be better.  -EINVAL should be all userspace needs under most circumstances.
> 
> Yes, please remove or degrade to dbg all the prints that userspace
> could trigger.

Sure

> 
>>> +   if (rpc->in.len > 0) {
>>> +           in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
>>> +           if (!in_payload) {
>>> +                   dev_err(dev, "Failed to allocate in_payload\n");
> 
> kzalloc is already super noisy if it fails

I know ... I get dinged on this all the time, but I still add these 
because there are multiple allocs in this one function and it isn't 
immediately obvious in the trace which alloc failed.  I could push each 
alloc off to separate functions to get that info in the trace, but it 
seems to me that's extra unnecessary code to get the same one more line.


> 
>>> +                   out = ERR_PTR(-ENOMEM);
>>> +                   goto done;
>>> +           }
>>> +
>>> +           if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
>>> +                              rpc->in.len)) {
>>> +                   dev_err(dev, "Failed to copy in_payload from user\n");
>>> +                   out = ERR_PTR(-EFAULT);
>>> +                   goto done;
>>> +           }
>>> +
>>> +           in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
>>> +                                                rpc->in.len, DMA_TO_DEVICE);
>>> +           err = dma_mapping_error(dev->parent, in_payload_dma_addr);
>>> +           if (err) {
>>> +                   dev_err(dev, "Failed to map in_payload\n");
>>> +                   in_payload_dma_addr = 0;
> 
> etc
> 
>>> +   err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>>> +   if (err) {
>>> +           dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
>>> +                   __func__, rpc->in.ep, rpc->in.op,
>>> +                   cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>>> +                   cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
>>> +                   err);
> 
> Triggerable by a malformed RPC?

That or misbehaving firmware.

> 
>>> +           out = ERR_PTR(err);
>>> +           goto done;
>>> +   }
>>> +
>>> +   dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
>>> +
>>> +   if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
>>> +           dev_err(dev, "Failed to copy out_payload to user\n");
> 
> Triggerable by a malformed user provided pointer

yes

> 
> Jason

Thanks,
sln