| Message ID | 20220517070527.10591-3-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | amt: fix several bugs in gateway mode | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 12 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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() ?
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@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() ? 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.
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");
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> --- v2: - Separate patch drivers/net/amt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)