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

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

[Dan Carpenter]

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?

  1698                  ptl_warn(ptl, "rx: parser: invalid start of frame, skipping\n");
  1699  
  1700                  /*
  1701                   * Notes:
  1702                   * - This might send multiple NAKs in case the communication

regards,
dan carpenter

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

[Maximilian Luz]

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?

Regards,
Max

> 
>    1698                  ptl_warn(ptl, "rx: parser: invalid start of frame, skipping\n");
>    1699
>    1700                  /*
>    1701                   * Notes:
>    1702                   * - This might send multiple NAKs in case the communication
> 
> regards,
> dan carpenter
> 

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

[Dan Carpenter]

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

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

[Maximilian Luz]

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
>