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. Why not just
make the condition:
if (aligned.ptr != source->ptr) {
Anyway, I assume you know what you're doing. :)
regards,
dan carepnter
