From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D56DC433F5 for ; Tue, 17 May 2022 22:24:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230078AbiEQWYN (ORCPT ); Tue, 17 May 2022 18:24:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49872 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229940AbiEQWYM (ORCPT ); Tue, 17 May 2022 18:24:12 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A877352B2F for ; Tue, 17 May 2022 15:24:11 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 448B061311 for ; Tue, 17 May 2022 22:24:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 71DB2C385B8; Tue, 17 May 2022 22:24:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1652826250; bh=ej3uXWDkh/DkN8vyF0PKjkKHGnNQ/EKjLTlxxU6AwdY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=b6kt+xpIzksdmv/CTfMsPVXaYYoofMYfE9zliOJPwSX/OLDYg2yzs1zm/4EddfP9C Xqu1t5zUUH9Xpd/gwDbQjRSgCSOqIMZ0Zo4EhmyB3C4kdzmghSaPJx61zY0QIaatjX 2wmC3HEUhH3V++CMIDLwNDM5gXjYzT1v9ydwRiS9UpsoRtT0zk7zCzV1wKeMj1QSji IUzJ7NQq7U6u9rVN4c8N9P8Rd8spwzWXGgsTO/ipCFYeHzG83p+RHfM94ApMSFolbN 05/64NoNCHM0Dw4kLimtmXs6XCLkMEt5HDjg4ThyIz1q7LHww5OSYVDf7UP3jghMPa R2WJ08X1UHvvg== Date: Tue, 17 May 2022 15:24:09 -0700 From: Jakub Kicinski To: Taehee Yoo 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 Message-ID: <20220517152409.5545f6e8@kernel.org> In-Reply-To: <20220517070527.10591-3-ap420073@gmail.com> References: <20220517070527.10591-1-ap420073@gmail.com> <20220517070527.10591-3-ap420073@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > 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() ?