Re: [bug report] platform/surface: Add Surface Aggregator subsystem

[Maximilian Luz <luzmaximilian@gmail.com>]

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
>