netdev - Re: [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220517152409.5545f6e8@kernel.org>
Date:   Tue, 17 May 2022 15:24:09 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net v2 2/2] amt: do not skip remaining handling of
 advertisement message

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@...il.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() ?