* [PATCH net v2 0/2] amt: fix several bugs in gateway mode @ 2022-05-17 7:05 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 0 siblings, 2 replies; 5+ messages in thread From: Taehee Yoo @ 2022-05-17 7:05 UTC (permalink / raw) To: davem, kuba, pabeni, edumazet, netdev; +Cc: ap420073 This patchset fixes bugs in amt module. First patch fixes amt gateway mode's status stuck. amt gateway and relay established so these two mode manage status. But gateway stuck to change its own status if a relay doesn't send responses. Second patch fixes a memory leak. an amt gateway skips some handling of advertisement message. So, a memory leak would occur. v2: - Separate patch - Add patch cover-letter Taehee Yoo (2): amt: fix gateway mode stuck amt: do not skip remaining handling of advertisement message drivers/net/amt.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net v2 1/2] amt: fix gateway mode stuck 2022-05-17 7:05 [PATCH net v2 0/2] amt: fix several bugs in gateway mode Taehee Yoo @ 2022-05-17 7:05 ` Taehee Yoo 2022-05-17 7:05 ` [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message Taehee Yoo 1 sibling, 0 replies; 5+ messages in thread From: Taehee Yoo @ 2022-05-17 7:05 UTC (permalink / raw) To: davem, kuba, pabeni, edumazet, netdev; +Cc: ap420073 If a gateway can not receive any response to requests from a relay, gateway resets status from SENT_REQUEST to INIT and variable about a relay as well. And then it should start the full establish step from sending a discovery message and receiving advertisement message. But, after failure in amt_req_work() it continues sending a request message step with flushed(invalid) relay information and sets SENT_REQUEST. So, a gateway can't be established with a relay. In order to avoid this situation, it stops sending the request message step if it fails. Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v2: - Separate patch. drivers/net/amt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/amt.c b/drivers/net/amt.c index 10455c9b9da0..2b4ce3869f08 100644 --- a/drivers/net/amt.c +++ b/drivers/net/amt.c @@ -943,7 +943,7 @@ static void amt_req_work(struct work_struct *work) if (amt->status < AMT_STATUS_RECEIVED_ADVERTISEMENT) goto out; - if (amt->req_cnt++ > AMT_MAX_REQ_COUNT) { + if (amt->req_cnt > AMT_MAX_REQ_COUNT) { netdev_dbg(amt->dev, "Gateway is not ready"); amt->qi = AMT_INIT_REQ_TIMEOUT; amt->ready4 = false; @@ -951,13 +951,15 @@ static void amt_req_work(struct work_struct *work) amt->remote_ip = 0; __amt_update_gw_status(amt, AMT_STATUS_INIT, false); amt->req_cnt = 0; + goto out; } spin_unlock_bh(&amt->lock); amt_send_request(amt, false); amt_send_request(amt, true); - amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true); spin_lock_bh(&amt->lock); + __amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true); + amt->req_cnt++; out: exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT); mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp * 1000)); -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message 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 ` Taehee Yoo 2022-05-17 22:24 ` Jakub Kicinski 1 sibling, 1 reply; 5+ messages in thread From: Taehee Yoo @ 2022-05-17 7:05 UTC (permalink / raw) To: davem, kuba, pabeni, edumazet, netdev; +Cc: ap420073 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(-) 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"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message 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 2022-05-18 0:21 ` Taehee Yoo 0 siblings, 1 reply; 5+ messages in thread From: Jakub Kicinski @ 2022-05-17 22:24 UTC (permalink / raw) To: Taehee Yoo; +Cc: davem, pabeni, edumazet, netdev 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() ? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2 2/2] amt: do not skip remaining handling of advertisement message 2022-05-17 22:24 ` Jakub Kicinski @ 2022-05-18 0:21 ` Taehee Yoo 0 siblings, 0 replies; 5+ messages in thread From: Taehee Yoo @ 2022-05-18 0:21 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, pabeni, edumazet, netdev 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-18 0:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 2022-05-18 0:21 ` Taehee Yoo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).