| 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
| ||
|
Message-ID: <3b837f52-580e-5ce2-5aa3-60c9d03a7520@gmail.com>
Date: Wed, 18 May 2022 09:21:34 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
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 5/18/22 07:24, Jakub Kicinski wrote:
Hi Jakub,
Thank s a lot for your review!
> 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() ?
I'm so sorry that I fully misunderstood your review and I found my
mistakes...
The rx_dropped was disappeared in my sight even though you pointed.
I think I tried to check only skb, not rx_dropped.
Now I fully understand your review and my mistake.
Powered by blists - more mailing lists