* [bug report] platform/surface: Add Surface Aggregator subsystem
@ 2021-01-25 11:42 Dan Carpenter
2021-01-25 12:12 ` Maximilian Luz
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-01-25 11:42 UTC (permalink / raw)
To: luzmaximilian; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] platform/surface: Add Surface Aggregator subsystem
2021-01-25 11:42 [bug report] platform/surface: Add Surface Aggregator subsystem Dan Carpenter
@ 2021-01-25 12:12 ` Maximilian Luz
2021-01-26 10:11 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Maximilian Luz @ 2021-01-25 12:12 UTC (permalink / raw)
To: Dan Carpenter; +Cc: platform-driver-x86
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] platform/surface: Add Surface Aggregator subsystem
2021-01-25 12:12 ` Maximilian Luz
@ 2021-01-26 10:11 ` Dan Carpenter
2021-01-26 15:45 ` Maximilian Luz
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-01-26 10:11 UTC (permalink / raw)
To: Maximilian Luz; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] platform/surface: Add Surface Aggregator subsystem
2021-01-26 10:11 ` Dan Carpenter
@ 2021-01-26 15:45 ` Maximilian Luz
0 siblings, 0 replies; 4+ messages in thread
From: Maximilian Luz @ 2021-01-26 15:45 UTC (permalink / raw)
To: Dan Carpenter; +Cc: platform-driver-x86
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-26 15:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 11:42 [bug report] platform/surface: Add Surface Aggregator subsystem Dan Carpenter
2021-01-25 12:12 ` Maximilian Luz
2021-01-26 10:11 ` Dan Carpenter
2021-01-26 15:45 ` Maximilian Luz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.