* [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
@ 2021-07-29 14:16 Dan Carpenter
2021-07-29 21:08 ` James Carlson
` (5 more replies)
0 siblings, 6 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-07-29 14:16 UTC (permalink / raw)
To: linux-ppp
[ This is ancient code, but the warning seems somewhat reasonable and
hopefully not too complicated to review? - dan ]
Hello PPP devs,
The patch 8a49ad6e89fe: "ppp: fix 'ppp_mp_reconstruct bad seq'
errors" from Feb 24, 2012, leads to the following static checker
warning:
drivers/net/ppp/ppp_generic.c:2840 ppp_mp_reconstruct()
error: dereferencing freed memory 'tail'
drivers/net/ppp/ppp_generic.c
2692 static struct sk_buff *
2693 ppp_mp_reconstruct(struct ppp *ppp)
2694 {
2695 u32 seq = ppp->nextseq;
2696 u32 minseq = ppp->minseq;
2697 struct sk_buff_head *list = &ppp->mrq;
2698 struct sk_buff *p, *tmp;
2699 struct sk_buff *head, *tail;
2700 struct sk_buff *skb = NULL;
2701 int lost = 0, len = 0;
2702
2703 if (ppp->mrru = 0) /* do nothing until mrru is set */
2704 return NULL;
2705 head = __skb_peek(list);
2706 tail = NULL;
2707 skb_queue_walk_safe(list, p, tmp) {
2708 again:
2709 if (seq_before(PPP_MP_CB(p)->sequence, seq)) {
2710 /* this can't happen, anyway ignore the skb */
2711 netdev_err(ppp->dev, "ppp_mp_reconstruct bad "
2712 "seq %u < %u\n",
2713 PPP_MP_CB(p)->sequence, seq);
2714 __skb_unlink(p, list);
2715 kfree_skb(p);
2716 continue;
2717 }
2718 if (PPP_MP_CB(p)->sequence != seq) {
2719 u32 oldseq;
2720 /* Fragment `seq' is missing. If it is after
2721 minseq, it might arrive later, so stop here. */
2722 if (seq_after(seq, minseq))
2723 break;
2724 /* Fragment `seq' is lost, keep going. */
2725 lost = 1;
2726 oldseq = seq;
2727 seq = seq_before(minseq, PPP_MP_CB(p)->sequence)?
2728 minseq + 1: PPP_MP_CB(p)->sequence;
2729
2730 if (ppp->debug & 1)
2731 netdev_printk(KERN_DEBUG, ppp->dev,
2732 "lost frag %u..%u\n",
2733 oldseq, seq-1);
2734
2735 goto again;
2736 }
2737
2738 /*
2739 * At this point we know that all the fragments from
2740 * ppp->nextseq to seq are either present or lost.
2741 * Also, there are no complete packets in the queue
2742 * that have no missing fragments and end before this
2743 * fragment.
2744 */
2745
2746 /* B bit set indicates this fragment starts a packet */
2747 if (PPP_MP_CB(p)->BEbits & B) {
2748 head = p;
2749 lost = 0;
2750 len = 0;
2751 }
2752
2753 len += p->len;
2754
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.
2765 break;
2766 }
2767 ppp->nextseq = seq + 1;
2768 }
2769
2770 /*
2771 * If this is the ending fragment of a packet,
2772 * and we haven't found a complete valid packet yet,
2773 * we can discard up to and including this fragment.
2774 */
2775 if (PPP_MP_CB(p)->BEbits & E) {
^^^^^^^^^^^^^^^^^^^^^^^^
If "tail" is set, can this condition be true?
2776 struct sk_buff *tmp2;
2777
2778 skb_queue_reverse_walk_from_safe(list, p, tmp2) {
2779 if (ppp->debug & 1)
2780 netdev_printk(KERN_DEBUG, ppp->dev,
2781 "discarding frag %u\n",
2782 PPP_MP_CB(p)->sequence);
2783 __skb_unlink(p, list);
2784 kfree_skb(p);
^^^^^^^^^^^^
On the first iteration through the loop then "p" is still set to "tail"
so this will free "tail", leading to problems down the line.
2785 }
2786 head = skb_peek(list);
2787 if (!head)
2788 break;
2789 }
2790 ++seq;
2791 }
2792
2793 /* If we have a complete packet, copy it all into one skb. */
2794 if (tail != NULL) {
2795 /* If we have discarded any fragments,
2796 signal a receive error. */
2797 if (PPP_MP_CB(head)->sequence != ppp->nextseq) {
2798 skb_queue_walk_safe(list, p, tmp) {
2799 if (p = head)
2800 break;
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);
2807 }
2808
2809 if (ppp->debug & 1)
2810 netdev_printk(KERN_DEBUG, ppp->dev,
2811 " missed pkts %u..%u\n",
2812 ppp->nextseq,
2813 PPP_MP_CB(head)->sequence-1);
2814 ++ppp->dev->stats.rx_dropped;
2815 ppp_receive_error(ppp);
2816 }
2817
2818 skb = head;
2819 if (head != tail) {
2820 struct sk_buff **fragpp = &skb_shinfo(skb)->frag_list;
2821 p = skb_queue_next(list, head);
2822 __skb_unlink(skb, list);
2823 skb_queue_walk_from_safe(list, p, tmp) {
2824 __skb_unlink(p, list);
2825 *fragpp = p;
2826 p->next = NULL;
2827 fragpp = &p->next;
2828
2829 skb->len += p->len;
2830 skb->data_len += p->len;
2831 skb->truesize += p->truesize;
2832
2833 if (p = tail)
2834 break;
2835 }
2836 } else {
2837 __skb_unlink(skb, list);
2838 }
2839
--> 2840 ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2841 }
2842
2843 return skb;
2844 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
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
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: James Carlson @ 2021-07-29 21:08 UTC (permalink / raw)
To: linux-ppp
On 7/29/21 10:16 AM, Dan Carpenter wrote:
> The patch 8a49ad6e89fe: "ppp: fix 'ppp_mp_reconstruct bad seq'
> errors" from Feb 24, 2012, leads to the following static checker
> warning:
>
> drivers/net/ppp/ppp_generic.c:2840 ppp_mp_reconstruct()
> error: dereferencing freed memory 'tail'
What's the static checker, and does it provide any deeper analysis of
the code path and branch assumptions involved?
> 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.
>
> 2765 break;
> 2766 }
> 2767 ppp->nextseq = seq + 1;
> 2768 }
> 2769
> 2770 /*
> 2771 * If this is the ending fragment of a packet,
> 2772 * and we haven't found a complete valid packet yet,
> 2773 * we can discard up to and including this fragment.
> 2774 */
> 2775 if (PPP_MP_CB(p)->BEbits & E) {
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> If "tail" is set, can this condition be true?
No. You can't get here at all if tail is set. Note the break at line
2765, which takes us out of the "skb_queue_walk_safe" iteration. That
takes us all the way down to line 2793.
> 2776 struct sk_buff *tmp2;
> 2777
> 2778 skb_queue_reverse_walk_from_safe(list, p, tmp2) {
> 2779 if (ppp->debug & 1)
> 2780 netdev_printk(KERN_DEBUG, ppp->dev,
> 2781 "discarding frag %u\n",
> 2782 PPP_MP_CB(p)->sequence);
> 2783 __skb_unlink(p, list);
> 2784 kfree_skb(p);
> ^^^^^^^^^^^^
> On the first iteration through the loop then "p" is still set to "tail"
> so this will free "tail", leading to problems down the line.
tail must be NULL here. Otherwise, we would have broken out of the loop
at line 2765.
Other than that the code is a bit hard to read, I don't see a problem here.
--
James Carlson 42.703N 71.076W <carlsonj@workingcode.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
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
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-07-30 8:48 UTC (permalink / raw)
To: linux-ppp
Hi James,
Thanks for your response. This is a new not yet published Smatch check.
I reported the bug wrong, it's complaining about the other kfree_skb().
See below.
Smatch understands about break statements. :P
On Thu, Jul 29, 2021 at 05:16:17PM +0300, Dan Carpenter wrote:
> [ This is ancient code, but the warning seems somewhat reasonable and
> hopefully not too complicated to review? - dan ]
>
> Hello PPP devs,
>
> The patch 8a49ad6e89fe: "ppp: fix 'ppp_mp_reconstruct bad seq'
> errors" from Feb 24, 2012, leads to the following static checker
> warning:
>
> drivers/net/ppp/ppp_generic.c:2840 ppp_mp_reconstruct()
> error: dereferencing freed memory 'tail'
>
> drivers/net/ppp/ppp_generic.c
> 2692 static struct sk_buff *
> 2693 ppp_mp_reconstruct(struct ppp *ppp)
> 2694 {
> 2695 u32 seq = ppp->nextseq;
> 2696 u32 minseq = ppp->minseq;
> 2697 struct sk_buff_head *list = &ppp->mrq;
> 2698 struct sk_buff *p, *tmp;
> 2699 struct sk_buff *head, *tail;
> 2700 struct sk_buff *skb = NULL;
> 2701 int lost = 0, len = 0;
> 2702
> 2703 if (ppp->mrru = 0) /* do nothing until mrru is set */
> 2704 return NULL;
> 2705 head = __skb_peek(list);
> 2706 tail = NULL;
> 2707 skb_queue_walk_safe(list, p, tmp) {
> 2708 again:
> 2709 if (seq_before(PPP_MP_CB(p)->sequence, seq)) {
> 2710 /* this can't happen, anyway ignore the skb */
> 2711 netdev_err(ppp->dev, "ppp_mp_reconstruct bad "
> 2712 "seq %u < %u\n",
> 2713 PPP_MP_CB(p)->sequence, seq);
> 2714 __skb_unlink(p, list);
> 2715 kfree_skb(p);
> 2716 continue;
> 2717 }
> 2718 if (PPP_MP_CB(p)->sequence != seq) {
> 2719 u32 oldseq;
> 2720 /* Fragment `seq' is missing. If it is after
> 2721 minseq, it might arrive later, so stop here. */
> 2722 if (seq_after(seq, minseq))
> 2723 break;
> 2724 /* Fragment `seq' is lost, keep going. */
> 2725 lost = 1;
> 2726 oldseq = seq;
> 2727 seq = seq_before(minseq, PPP_MP_CB(p)->sequence)?
> 2728 minseq + 1: PPP_MP_CB(p)->sequence;
> 2729
> 2730 if (ppp->debug & 1)
> 2731 netdev_printk(KERN_DEBUG, ppp->dev,
> 2732 "lost frag %u..%u\n",
> 2733 oldseq, seq-1);
> 2734
> 2735 goto again;
> 2736 }
> 2737
> 2738 /*
> 2739 * At this point we know that all the fragments from
> 2740 * ppp->nextseq to seq are either present or lost.
> 2741 * Also, there are no complete packets in the queue
> 2742 * that have no missing fragments and end before this
> 2743 * fragment.
> 2744 */
> 2745
> 2746 /* B bit set indicates this fragment starts a packet */
> 2747 if (PPP_MP_CB(p)->BEbits & B) {
> 2748 head = p;
> 2749 lost = 0;
> 2750 len = 0;
> 2751 }
> 2752
> 2753 len += p->len;
> 2754
> 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.
>
> 2765 break;
We hit the break statement.
> 2766 }
> 2767 ppp->nextseq = seq + 1;
> 2768 }
> 2769
> 2770 /*
> 2771 * If this is the ending fragment of a packet,
> 2772 * and we haven't found a complete valid packet yet,
> 2773 * we can discard up to and including this fragment.
> 2774 */
> 2775 if (PPP_MP_CB(p)->BEbits & E) {
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> If "tail" is set, can this condition be true?
>
> 2776 struct sk_buff *tmp2;
> 2777
> 2778 skb_queue_reverse_walk_from_safe(list, p, tmp2) {
> 2779 if (ppp->debug & 1)
> 2780 netdev_printk(KERN_DEBUG, ppp->dev,
> 2781 "discarding frag %u\n",
> 2782 PPP_MP_CB(p)->sequence);
> 2783 __skb_unlink(p, list);
> 2784 kfree_skb(p);
> ^^^^^^^^^^^^
> On the first iteration through the loop then "p" is still set to "tail"
> so this will free "tail", leading to problems down the line.
I was just completely wrong to write this.
>
> 2785 }
> 2786 head = skb_peek(list);
> 2787 if (!head)
> 2788 break;
> 2789 }
> 2790 ++seq;
> 2791 }
We jump to here.
> 2792
> 2793 /* If we have a complete packet, copy it all into one skb. */
> 2794 if (tail != NULL) {
This condition means "tail = p"
> 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.
> 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.
> 2800 break;
> 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.
> 2807 }
> 2808
> 2809 if (ppp->debug & 1)
> 2810 netdev_printk(KERN_DEBUG, ppp->dev,
> 2811 " missed pkts %u..%u\n",
> 2812 ppp->nextseq,
> 2813 PPP_MP_CB(head)->sequence-1);
> 2814 ++ppp->dev->stats.rx_dropped;
> 2815 ppp_receive_error(ppp);
> 2816 }
> 2817
> 2818 skb = head;
> 2819 if (head != tail) {
> 2820 struct sk_buff **fragpp = &skb_shinfo(skb)->frag_list;
> 2821 p = skb_queue_next(list, head);
> 2822 __skb_unlink(skb, list);
> 2823 skb_queue_walk_from_safe(list, p, tmp) {
> 2824 __skb_unlink(p, list);
> 2825 *fragpp = p;
> 2826 p->next = NULL;
> 2827 fragpp = &p->next;
> 2828
> 2829 skb->len += p->len;
> 2830 skb->data_len += p->len;
> 2831 skb->truesize += p->truesize;
> 2832
> 2833 if (p = tail)
> 2834 break;
> 2835 }
> 2836 } else {
> 2837 __skb_unlink(skb, list);
> 2838 }
> 2839
> --> 2840 ppp->nextseq = PPP_MP_CB(tail)->sequence + 1;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is where Smatch complains.
>
> 2841 }
> 2842
> 2843 return skb;
> 2844 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
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
2021-07-31 18:36 ` James Carlson
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: James Carlson @ 2021-07-30 17:15 UTC (permalink / raw)
To: linux-ppp
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
2021-07-29 14:16 [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors Dan Carpenter
` (2 preceding siblings ...)
2021-07-30 17:15 ` James Carlson
@ 2021-07-31 18:36 ` James Carlson
2021-08-02 11:43 ` Dan Carpenter
2021-08-02 12:37 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: James Carlson @ 2021-07-31 18:36 UTC (permalink / raw)
To: linux-ppp
On 7/30/21 1:15 PM, James Carlson wrote:
> On 7/30/21 4:48 AM, Dan Carpenter wrote:
>>> --> 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.
For what it's worth, it's not my code, and I agree that it's at least a
bit hard to follow, and may well harbor bugs. If you're suggesting
either some kind of suppression directive (I tried looking for some
Smatch documentation but couldn't find much to suggest that's possible,
though I guess now that you'd be the one who knows for sure), or adding
something like "u32 nextseq = PPP_CB(tail)->sequence + 1;" around line
2795, and then using "ppp->nextseq = nextseq;" on 2840, then I'd be in
favor of that, at least to make the tool happy.
Is there an equivalent of "/* LINTED: tail pointer still ok */" here?
--
James Carlson 42.703N 71.076W <carlsonj@workingcode.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
2021-07-29 14:16 [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors Dan Carpenter
` (3 preceding siblings ...)
2021-07-31 18:36 ` James Carlson
@ 2021-08-02 11:43 ` Dan Carpenter
2021-08-02 12:37 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-08-02 11:43 UTC (permalink / raw)
To: linux-ppp
On Fri, Jul 30, 2021 at 01:15:39PM -0400, James Carlson wrote:
> >> 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.'
Crud... I can't believe I misread this code twice.
I'm not actually sure why Smatch doesn't get this correct. I wanted to
blame it on the new unpublished bits but even the published code is
buggy. :/ I will investigate and fix this.
Thanks for taking the time on this.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors
2021-07-29 14:16 [bug report] ppp: fix 'ppp_mp_reconstruct bad seq' errors Dan Carpenter
` (4 preceding siblings ...)
2021-08-02 11:43 ` Dan Carpenter
@ 2021-08-02 12:37 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-08-02 12:37 UTC (permalink / raw)
To: linux-ppp
On Sat, Jul 31, 2021 at 02:36:03PM -0400, James Carlson wrote:
> On 7/30/21 1:15 PM, James Carlson wrote:
> > On 7/30/21 4:48 AM, Dan Carpenter wrote:
> > > > --> 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.
>
> For what it's worth, it's not my code, and I agree that it's at least a bit
> hard to follow, and may well harbor bugs. If you're suggesting either some
> kind of suppression directive (I tried looking for some Smatch documentation
> but couldn't find much to suggest that's possible, though I guess now that
> you'd be the one who knows for sure), or adding something like "u32 nextseq
> = PPP_CB(tail)->sequence + 1;" around line 2795, and then using
> "ppp->nextseq = nextseq;" on 2840, then I'd be in favor of that, at least to
> make the tool happy.
No. No. Never change the code just to make the tool happy. Of course,
I misread this in two different ways because the first time I didn't
spot the break statement and the second time I got skb_queue_walk_safe()
and skb_queue_walk_from_safe(). But it's not really hard to read if
you're more familiar with those macros.
I've investigated this and it turns out the problem is a kind of known
issue with how Smatch parses lists. I've avoided fixing this for years
because parsing lists correctly will be a big slow down and it's a
quite a big project but probably I should just fix it. Maybe later this
year.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-02 12:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2021-07-31 18:36 ` James Carlson
2021-08-02 11:43 ` Dan Carpenter
2021-08-02 12:37 ` Dan Carpenter
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.