On 1/26/21 11:11 AM, Dan Carpenter wrote:
On Mon, Jan 25, 2021 at 01:12:06PM +0100, Maximilian Luz wrote:
On 1/25/21 12:42 PM, Dan Carpenter wrote:
Hello Maximilian Luz,
The patch c167b9c7e3d6: "platform/surface: Add Surface Aggregator
subsystem" from Dec 21, 2020, leads to the following static checker
warning:
drivers/platform/surface/aggregator/ssh_packet_layer.c:1697 ssh_ptl_rx_eval()
warn: check likely/unlikely parens
drivers/platform/surface/aggregator/ssh_packet_layer.c
1683 static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
1684 {
1685 struct ssh_frame *frame;
1686 struct ssam_span payload;
1687 struct ssam_span aligned;
1688 bool syn_found;
1689 int status;
1690
1691 /* Error injection: Modify data to simulate corrupt SYN bytes. */
1692 ssh_ptl_rx_inject_invalid_syn(ptl, source);
1693
1694 /* Find SYN. */
1695 syn_found = sshp_find_syn(source, &aligned);
1696
1697 if (unlikely(aligned.ptr - source->ptr) > 0) {
The unlikely() macro returns 0/1. Smatch is suggesting that this should
just be "if (unlikely((aligned.ptr - source->ptr) > 0)) {" but I'm not
at all sure that that's correct. Isn't aligned being higher than source
the normal case?
Hi,
the suggestion by Smatch, i.e.
if (unlikely((aligned.ptr - source->ptr) > 0)
should be correct. The normal case is aligned.ptr equal to source->ptr.
Let me elaborate a bit for detail: SSH messages all start with a "SYN"
sequence and are frame based. So once we've parsed one message, we
expect it to be followed up directly by the next message. So, here, we
are directly expecting a new message to start, meaning the SYN should be
found at the start of the buffer, or, closer to the code, aligned.ptr
(the start of the message frame) should equal source->ptr. If this is
not the case, this indicates that some unexpected bytes are in-between
the last successfully parsed message and the next message. Usually
that's caused by when a previous message has failed one of the CRC
checks (thus we can't rely on the length encoded in the frame) and we're
actively searching for the next message (via this call here).
Thanks for the report!
Do you want to submit a patch for this or should I do that?
I think I understand, but can you send the patch for this.
Sure, I'll do that. Expect it later today.
Why not just make the condition:
if (aligned.ptr != source->ptr) {
The '!=' would work too. The (frame-)aligned pointer is guaranteed to
always be either equal to or greater than the source pointer, so that's
equivalent to '>'. I'd kind of like to keep the unlikely though, as that
indicates it's part of the failure path and it's a failure we don't
expect frequently.
That section could probably also use a comment.
Anyway, I assume you know what you're doing. :)
regards,
dan carepnter
