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
