[PATCH net v2 0/2] amt: fix several bugs in gateway mode

[PATCH net v2 0/2] amt: fix several bugs in gateway mode

[Taehee Yoo]

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

[PATCH net v2 1/2] amt: fix gateway mode stuck

[Taehee Yoo]

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

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

[Taehee Yoo]

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

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

[Jakub Kicinski]

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() ?

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

[Taehee Yoo]

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.