All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chenyuan Yang <chenyuan0y@gmail.com>
To: chandrashekar.devegowda@intel.com,
	chiranjeevi.rapolu@linux.intel.com, haijun.liu@mediatek.com,
	m.chetan.kumar@linux.intel.com, ricardo.martinez@linux.intel.com,
	loic.poulain@linaro.org, ryazanov.s.a@gmail.com,
	johannes@sipsolutions.net, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com
Cc: netdev@vger.kernel.org, zzjas98@gmail.com
Subject: [drivers/net/wwan] Question about possible memory leaks in t7xx_dpmaif_rx_buf_alloc() and t7xx_dpmaif_rx_frag_alloc()
Date: Thu, 14 Mar 2024 20:16:09 -0500	[thread overview]
Message-ID: <ZfOhWVnTsE8JAhXk@cy-server> (raw)

Dear WWAN Driver Developers,

We are curious whether the functions `t7xx_dpmaif_rx_buf_alloc()` and `t7xx_dpmaif_rx_frag_alloc` might have memory leaks.

#1. The function `t7xx_dpmaif_rx_buf_alloc` is https://elixir.bootlin.com/linux/v6.8/source/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c#L164
and the relevant code is
```
int t7xx_dpmaif_rx_buf_alloc(struct dpmaif_ctrl *dpmaif_ctrl,
			     const struct dpmaif_bat_request *bat_req,
			     const unsigned int q_num, const unsigned int buf_cnt,
			     const bool initial)
{
	unsigned int i, bat_cnt, bat_max_cnt, bat_start_idx;
	int ret;
	...
	for (i = 0; i < buf_cnt; i++) {
		unsigned int cur_bat_idx = bat_start_idx + i;
		struct dpmaif_bat_skb *cur_skb;
		struct dpmaif_bat *cur_bat;

		if (cur_bat_idx >= bat_max_cnt)
			cur_bat_idx -= bat_max_cnt;

		cur_skb = (struct dpmaif_bat_skb *)bat_req->bat_skb + cur_bat_idx;
		if (!cur_skb->skb &&
		    !t7xx_alloc_and_map_skb_info(dpmaif_ctrl, bat_req->pkt_buf_sz, cur_skb))
			break;

		cur_bat = (struct dpmaif_bat *)bat_req->bat_base + cur_bat_idx;
	}
	...
	ret = t7xx_dpmaif_update_bat_wr_idx(dpmaif_ctrl, q_num, i);
	if (ret)
		goto err_unmap_skbs;
	...
err_unmap_skbs:
	while (--i > 0)
		t7xx_unmap_bat_skb(dpmaif_ctrl->dev, bat_req->bat_skb, i);
}
```

In the label `err_unmap_skbs`, the function will unmap the allocated memory for `bat_req->bat_skb` by checking `while (--i > 0)`. However, the first element (`i=0`) of `bat_req->bat_skb` is not unmapped since `i` is decremented before the check. 
By contrast, in the function `t7xx_dpmaif_bat_free` (https://elixir.bootlin.com/linux/v6.8/source/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c#L984), the first element of `bat_req->bat_skb` is unmapped.

Based on our understanding, a possible fix would be
```
-      while (--i > 0)
+      while (--i >= 0)
```

#2. For another function `t7xx_dpmaif_rx_frag_alloc`, the function is https://elixir.bootlin.com/linux/v6.8/source/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c#L319

```
int t7xx_dpmaif_rx_frag_alloc(struct dpmaif_ctrl *dpmaif_ctrl, struct dpmaif_bat_request *bat_req,
			      const unsigned int buf_cnt, const bool initial)
{
	unsigned int buf_space, cur_bat_idx = bat_req->bat_wr_idx;
	struct dpmaif_bat_page *bat_skb = bat_req->bat_skb;
	int ret = 0, i;

	if (!buf_cnt || buf_cnt > bat_req->bat_size_cnt)
		return -EINVAL;
	...
	for (i = 0; i < buf_cnt; i++) {
		struct dpmaif_bat_page *cur_page = bat_skb + cur_bat_idx;
		struct dpmaif_bat *cur_bat;
		dma_addr_t data_base_addr;
		...
		cur_bat = (struct dpmaif_bat *)bat_req->bat_base + cur_bat_idx;
		cur_bat->buffer_addr_ext = upper_32_bits(data_base_addr);
		cur_bat->p_buffer_addr = lower_32_bits(data_base_addr);
		cur_bat_idx = t7xx_ring_buf_get_next_wr_idx(bat_req->bat_size_cnt, cur_bat_idx);
	}
	...
	if (i < buf_cnt) {
		ret = -ENOMEM;
		if (initial) {
			while (--i > 0)
				t7xx_unmap_bat_page(dpmaif_ctrl->dev, bat_req->bat_skb, i);
		}
	}

	return ret;
}
```

Similarly, the function will unmap the allocated memory for `bat_req->bat_skb` by checking `while (--i > 0)`. However, the first element (`i=0`) of `bat_req->bat_skb` is not unmapped since `i` is decremented before the check.

Please kindly correct us if we missed any key information. Looking forward to your response!

Best,
Chenyuan

                 reply	other threads:[~2024-03-15  1:16 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZfOhWVnTsE8JAhXk@cy-server \
    --to=chenyuan0y@gmail.com \
    --cc=chandrashekar.devegowda@intel.com \
    --cc=chiranjeevi.rapolu@linux.intel.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=haijun.liu@mediatek.com \
    --cc=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=m.chetan.kumar@linux.intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=ricardo.martinez@linux.intel.com \
    --cc=ryazanov.s.a@gmail.com \
    --cc=zzjas98@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.