| Message ID | 20220514131346.17045-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] amt: fix gateway mode stuck | expand |
On Sat, 14 May 2022 13:13:46 +0000 Taehee Yoo wrote: > - if (amt_advertisement_handler(amt, skb)) > + err = amt_advertisement_handler(amt, skb); > + if (err) > amt->dev->stats.rx_dropped++; > - goto out; > + break; There's another amt->dev->stats.rx_dropped++; before the end of this function which now won't be skipped, I think you're counting twice.
On 5/17/22 08:10, Jakub Kicinski wrote: Hi Jakub, Thanks a lot for your review! > On Sat, 14 May 2022 13:13:46 +0000 Taehee Yoo wrote: >> - if (amt_advertisement_handler(amt, skb)) >> + err = amt_advertisement_handler(amt, skb); >> + if (err) >> amt->dev->stats.rx_dropped++; >> - goto out; >> + break; > > There's another amt->dev->stats.rx_dropped++; before the end of this > function which now won't be skipped, I think you're counting twice. This is intended. It skips a remaining handling of advertisement message. So, I think a memory leak would occur at this point, so I added. Thanks! Taehee Yoo
diff --git a/drivers/net/amt.c b/drivers/net/amt.c index 10455c9b9da0..6ce2ecd07640 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)); @@ -2696,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");
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> --- drivers/net/amt.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)