From: Jakub Kicinski <kuba@kernel.org>
To: Taehee Yoo <ap420073@gmail.com>
Cc: davem@davemloft.net, pabeni@redhat.com, edumazet@google.com,
netdev@vger.kernel.org
Subject: Re: [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message
Date: Tue, 17 May 2022 15:24:09 -0700 [thread overview]
Message-ID: <20220517152409.5545f6e8@kernel.org> (raw)
In-Reply-To: <20220517070527.10591-3-ap420073@gmail.com>
On Tue, 17 May 2022 07:05:27 +0000 Taehee Yoo wrote:
> When a gateway receives an advertisement message, it extracts relay
> information and then it should be deleted.
> But the advertisement handler doesn't do that.
> So, after amt_advertisement_handler(), that message should not be skipped
> remaining handling.
>
> Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 2b4ce3869f08..6ce2ecd07640 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -2698,9 +2698,10 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
> err = true;
> goto drop;
> }
> - if (amt_advertisement_handler(amt, skb))
> + err = amt_advertisement_handler(amt, skb);
> + if (err)
> amt->dev->stats.rx_dropped++;
> - goto out;
> + break;
> case AMT_MSG_MULTICAST_DATA:
> if (iph->saddr != amt->remote_ip) {
> netdev_dbg(amt->dev, "Invalid Relay IP\n");
I guess I'll have to spell it out for you more cause either you didn't
understand me or I don't understand your reply on v1. Here's the full
function:
2669 static int amt_rcv(struct sock *sk, struct sk_buff *skb)
2670 {
2671 struct amt_dev *amt;
2672 struct iphdr *iph;
2673 int type;
2674 bool err;
2675
2676 rcu_read_lock_bh();
2677 amt = rcu_dereference_sk_user_data(sk);
2678 if (!amt) {
2679 err = true;
2680 goto out;
2681 }
2682
2683 skb->dev = amt->dev;
2684 iph = ip_hdr(skb);
2685 type = amt_parse_type(skb);
2686 if (type == -1) {
2687 err = true;
2688 goto drop;
2689 }
2690
2691 if (amt->mode == AMT_MODE_GATEWAY) {
2692 switch (type) {
2693 case AMT_MSG_ADVERTISEMENT:
2694 if (iph->saddr != amt->discovery_ip) {
2695 netdev_dbg(amt->dev, "Invalid Relay IP\n");
2696 err = true;
2697 goto drop;
2698 }
2699 if (amt_advertisement_handler(amt, skb))
2700 amt->dev->stats.rx_dropped++;
2701 goto out;
2702 case AMT_MSG_MULTICAST_DATA:
2703 if (iph->saddr != amt->remote_ip) {
2704 netdev_dbg(amt->dev, "Invalid Relay IP\n");
2705 err = true;
2706 goto drop;
2707 }
2708 err = amt_multicast_data_handler(amt, skb);
2709 if (err)
2710 goto drop;
2711 else
2712 goto out;
2713 case AMT_MSG_MEMBERSHIP_QUERY:
2714 if (iph->saddr != amt->remote_ip) {
2715 netdev_dbg(amt->dev, "Invalid Relay IP\n");
2716 err = true;
2717 goto drop;
2718 }
2719 err = amt_membership_query_handler(amt, skb);
2720 if (err)
2721 goto drop;
2722 else
2723 goto out;
2724 default:
2725 err = true;
2726 netdev_dbg(amt->dev, "Invalid type of Gateway\n");
2727 break;
2728 }
2729 } else {
2730 switch (type) {
2731 case AMT_MSG_DISCOVERY:
2732 err = amt_discovery_handler(amt, skb);
2733 break;
2734 case AMT_MSG_REQUEST:
2735 err = amt_request_handler(amt, skb);
2736 break;
2737 case AMT_MSG_MEMBERSHIP_UPDATE:
2738 err = amt_update_handler(amt, skb);
2739 if (err)
2740 goto drop;
2741 else
2742 goto out;
2743 default:
2744 err = true;
2745 netdev_dbg(amt->dev, "Invalid type of relay\n");
2746 break;
2747 }
2748 }
2749 drop:
2750 if (err) {
2751 amt->dev->stats.rx_dropped++;
2752 kfree_skb(skb);
2753 } else {
2754 consume_skb(skb);
2755 }
2756 out:
2757 rcu_read_unlock_bh();
2758 return 0;
2759 }
You're changing line 2699, we used to bump the rx_dropped on line 2700
and then the goto on line 2701 takes us to line 2756 - unlock, return.
Now since you have replaced the goto on line 2701 with a "break" the
code will go from line 2701 to line 2749/2750. If err is set we'll
increase rx_dropped again on line 2751.
In other words rx_dropped will be increased both on line 2700 and
line 2751.
What am I missing?
Also I don't quite understand your commit message. The only thing we
used to skip is the freeing of the skb. Or do you mean we need to
return an error from amt_rcv() ?
next prev parent reply other threads:[~2022-05-17 22:24 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-17 7:05 [PATCH net v2 0/2] amt: fix several bugs in gateway mode Taehee Yoo
2022-05-17 7:05 ` [PATCH net v2 1/2] amt: fix gateway mode stuck Taehee Yoo
2022-05-17 7:05 ` [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message Taehee Yoo
2022-05-17 22:24 ` Jakub Kicinski [this message]
2022-05-18 0:21 ` Taehee Yoo
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=20220517152409.5545f6e8@kernel.org \
--to=kuba@kernel.org \
--cc=ap420073@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/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.