| Message ID | 20241028-ncsi-arb-opcode-v1-1-9d65080908b9@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ncsi: check for netlink-driven responses before requiring a handler | expand |
On Mon, Oct 28, 2024 at 03:08:34PM +0800, Jeremy Kerr wrote: > Currently, the NCSI response path will look up an opcode-specific > handler for all incoming response messages. However, we may be receiving > a response from a netlink-generated request, which may not have a > corresponding in-kernel handler for that request opcode. In that case, > we'll drop the response because we didn't find a opcode-specific > handler. > > Perform the lookup for the pending request (and hence for > NETLINK_DRIVEN) before requiring an in-kernel handler, and defer the > requirement for a corresponding kernel request until we know it's a > kernel-driven command. > > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> Hi Jeremy, Not strictly related to this patch, but I do wonder if the log messages should be rate limited, which doesn't seem to be the case at this time. Regardless, this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org> ...
On Mon, 28 Oct 2024 15:08:34 +0800 Jeremy Kerr wrote: > Subject: [PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler > Currently, the NCSI response path will look up an opcode-specific > handler for all incoming response messages. However, we may be receiving > a response from a netlink-generated request, which may not have a > corresponding in-kernel handler for that request opcode. In that case, > we'll drop the response because we didn't find a opcode-specific > handler. This makes it sound like the code is written this way unintentionally, which I doubt. A better description of the patch would be "allow userspace to issue commands unknown to the kernel". And then it'd be great to get some examples of commands you'd like to issue.. > Perform the lookup for the pending request (and hence for > NETLINK_DRIVEN) before requiring an in-kernel handler, and defer the > requirement for a corresponding kernel request until we know it's a > kernel-driven command. As for the code - delaying handling ret != 0 makes me worried that someone will insert code in between and clobber it. Can you split the handling so that all the ret != 0 (or EPERM for netlink) are still handled in the if (ret) {} ?
Hi Jakub, > > Currently, the NCSI response path will look up an opcode-specific > > handler for all incoming response messages. However, we may be > > receiving > > a response from a netlink-generated request, which may not have a > > corresponding in-kernel handler for that request opcode. In that > > case, > > we'll drop the response because we didn't find a opcode-specific > > handler. > > This makes it sound like the code is written this way > unintentionally, which I doubt. This seems like an oversight in the response path, failing on a missing in-kernel handler even if we don't later use it. If we were intentionally enforcing only known commands, then we'd also be checking on the request side. But yes, I can certainly word this in terms of what this change now enables, and provide examples. > > Perform the lookup for the pending request (and hence for > > NETLINK_DRIVEN) before requiring an in-kernel handler, and defer > > the > > requirement for a corresponding kernel request until we know it's a > > kernel-driven command. > > As for the code - delaying handling ret != 0 makes me worried that > someone will insert code in between and clobber it. Can you split > the handling so that all the ret != 0 (or EPERM for netlink) > are still handled in the if (ret) {} ? Can do! The -EPERM case can probably be simplified a little too, as it just indicates we didn't get a success response from the remote NCSI device. Will work on a v2 soon. Cheers, Jeremy
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c index e28be33bdf2c487c0fbfe3a1b4de6f52c8f923cc..882767c138895eba5bd7e413b7edb6480d5807bf 100644 --- a/net/ncsi/ncsi-rsp.c +++ b/net/ncsi/ncsi-rsp.c @@ -1205,12 +1205,6 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev, } } - if (!nrh) { - netdev_err(nd->dev, "Received unrecognized packet (0x%x)\n", - hdr->type); - return -ENOENT; - } - /* Associate with the request */ spin_lock_irqsave(&ndp->lock, flags); nr = &ndp->requests[hdr->id]; @@ -1228,43 +1222,42 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev, /* Validate the packet */ spin_unlock_irqrestore(&ndp->lock, flags); - payload = nrh->payload; + payload = nrh ? nrh->payload : -1; if (payload < 0) payload = ntohs(hdr->length); + ret = ncsi_validate_rsp_pkt(nr, payload); - if (ret) { + if (ret) netdev_warn(ndp->ndev.dev, "NCSI: 'bad' packet ignored for type 0x%x\n", hdr->type); - if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) { - if (ret == -EPERM) - goto out_netlink; - else - ncsi_send_netlink_err(ndp->ndev.dev, - nr->snd_seq, - nr->snd_portid, - &nr->nlhdr, - ret); - } - goto out; - } - - /* Process the packet */ - ret = nrh->handler(nr); - if (ret) - netdev_err(ndp->ndev.dev, - "NCSI: Handler for packet type 0x%x returned %d\n", - hdr->type, ret); - -out_netlink: if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) { - ret = ncsi_rsp_handler_netlink(nr); - if (ret) { - netdev_err(ndp->ndev.dev, - "NCSI: Netlink handler for packet type 0x%x returned %d\n", - hdr->type, ret); + /* netlink driven: forward response to netlink socket */ + if (!ret || ret == -EPERM) + ncsi_rsp_handler_netlink(nr); + else + ncsi_send_netlink_err(ndp->ndev.dev, + nr->snd_seq, + nr->snd_portid, + &nr->nlhdr, + ret); + } else if (nrh) { + /* not netlink driven: process the packet in the kernel. We + * need to have a handler for this + */ + if (!ret) { + ret = nrh->handler(nr); + if (ret) + netdev_err(ndp->ndev.dev, + "NCSI: Handler for packet type 0x%x returned %d\n", + hdr->type, ret); } + + } else { + netdev_err(nd->dev, "Received unrecognized packet (0x%x)\n", + hdr->type); + ret = -ENOENT; } out:
Currently, the NCSI response path will look up an opcode-specific handler for all incoming response messages. However, we may be receiving a response from a netlink-generated request, which may not have a corresponding in-kernel handler for that request opcode. In that case, we'll drop the response because we didn't find a opcode-specific handler. Perform the lookup for the pending request (and hence for NETLINK_DRIVEN) before requiring an in-kernel handler, and defer the requirement for a corresponding kernel request until we know it's a kernel-driven command. Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- net/ncsi/ncsi-rsp.c | 61 ++++++++++++++++++++++++----------------------------- 1 file changed, 27 insertions(+), 34 deletions(-) --- base-commit: 03fc07a24735e0be8646563913abf5f5cb71ad19 change-id: 20241028-ncsi-arb-opcode-a346ddb88862 Best regards,