From: James Carlson <carlsonj@workingcode.com>
To: linux-ppp@vger.kernel.org
Subject: Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
Date: Fri, 30 Jul 2021 17:15:39 +0000 [thread overview]
Message-ID: <6c6f81af-db62-6644-117a-3bf0a1d62087@workingcode.com> (raw)
In-Reply-To: <20210729141617.GC1267@kili>
On 7/30/21 4:48 AM, Dan Carpenter wrote:
>> 2755 /* Got a complete packet yet? */
>> 2756 if (lost = 0 && (PPP_MP_CB(p)->BEbits & E) &&
>> 2757 (PPP_MP_CB(head)->BEbits & B)) {
>> 2758 if (len > ppp->mrru + 2) {
>> 2759 ++ppp->dev->stats.rx_length_errors;
>> 2760 netdev_printk(KERN_DEBUG, ppp->dev,
>> 2761 "PPP: reconstructed packet"
>> 2762 " is too long (%d)\n", len);
>> 2763 } else {
>> 2764 tail = p;
>> ^^^^^^^^
>> tail is set to p.
>
> At this point Smatch understands that "tail" and "p" are non-NULL.
Yep. And 'head' is non-NULL and points to the first buf of the
reassembled packet, 'tail' is non-NULL and points to the last buf of the
reassembled packet. And head may be equal to tail if it's packet
consisting of a single MP fragment. And because 'lost' is zero, we know
that we have all of the intermediate fragments chained as well. It's a
complete message.
>> 2793 /* If we have a complete packet, copy it all into one skb. */
>> 2794 if (tail != NULL) {
>
> This condition means "tail = p"
True at this point. (Not real meaningful, as we'll see in a bit, but
true nonetheless.)
>> 2795 /* If we have discarded any fragments,
>> 2796 signal a receive error. */
>> 2797 if (PPP_MP_CB(head)->sequence != ppp->nextseq) {
>
> Smatch is supposed to "understand" condtions, but this one is quite
> complicated and the only thing that Smatch understands is just the
> basic meaning that these two are not equal.
That's ok; it's a worthwhile branch to explore, so we can assume it's true.
>> 2798 skb_queue_walk_safe(list, p, tmp) {
>> 2799 if (p = head)
>
> One of the weak points of Smatch is how it parses lists... Also it
> doesn't have any implications for this if (p = head) condition.
This is where things break down. That queue walker macro on line 2798
re-assigns 'p'. The code marches over the list and says "anything that
still exists up to (but not including) the head for this completed
packet is trash." Note that *NOTHING* here is harming 'head' or
anything in the list that follows that buffer -- which includes 'tail.'
>> 2800 break;
That break protects us from hurting 'tail'.
>> 2801 if (ppp->debug & 1)
>> 2802 netdev_printk(KERN_DEBUG, ppp->dev,
>> 2803 "discarding frag %u\n",
>> 2804 PPP_MP_CB(p)->sequence);
>> 2805 __skb_unlink(p, list);
>> 2806 kfree_skb(p);
>
> We know that p = tail going in to the start of this list so this is
> going to free tail. Of course kfree_skb() is refcounted and the free
> only happens when the last reference is dropped.
Not so. p != tail here. It cannot possibly be tail, because we (A)
reassigned 'p' at the top of the loop and (B) broke out of the loop on
hitting 'head'.
>> 2836 } else {
>> 2837 __skb_unlink(skb, list);
>> 2838 }
>> 2839
>> --> 2840 ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Here is where Smatch complains.
If that's Smatch's analysis of the situation, then Smatch is wrong.
It's a bogus warning.
--
James Carlson 42.703N 71.076W <carlsonj@workingcode.com>
next prev parent reply other threads:[~2021-07-30 17:15 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 14:16 [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors Dan Carpenter
2021-07-29 21:08 ` James Carlson
2021-07-30 8:48 ` Dan Carpenter
2021-07-30 17:15 ` James Carlson [this message]
2021-07-31 18:36 ` James Carlson
2021-08-02 11:43 ` Dan Carpenter
2021-08-02 12:37 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6c6f81af-db62-6644-117a-3bf0a1d62087@workingcode.com \
--to=carlsonj@workingcode.com \
--cc=linux-ppp@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.