[PATCH net] net: ipa: lock when freeing transaction

[PATCH net] net: ipa: lock when freeing transaction

[Alex Elder]

Transactions sit on one of several lists, depending on their state
(allocated, pending, complete, or polled).  A spinlock protects
against concurrent access when transactions are moved between these
lists.

Transactions are also reference counted.  A newly-allocated
transaction has an initial count of 1; a transaction is released in
gsi_trans_free() only if its decremented reference count reaches 0.
Releasing a transaction includes removing it from the polled (or if
unused, allocated) list, so the spinlock is acquired when we release
a transaction.

The reference count is used to allow a caller to synchronously wait
for a committed transaction to complete.  In this case, the waiter
takes an extra reference to the transaction *before* committing it
(so it won't be freed), and releases its reference (calls
gsi_trans_free()) when it is done with it.

Similarly, gsi_channel_update() takes an extra reference to ensure a
transaction isn't released before the function is done operating on
it.  Until the transaction is moved to the completed list (by this
function) it won't be freed, so this reference is taken "safely."

But in the quiesce path, we want to wait for the "last" transaction,
which we find in the completed or polled list.  Transactions on
these lists can be freed at any time, so we (try to) prevent that
by taking the reference while holding the spinlock.

Currently gsi_trans_free() decrements a transaction's reference
count unconditionally, acquiring the lock to remove the transaction
from its list *only* when the count reaches 0.  This does not
protect the quiesce path, which depends on the lock to ensure its
extra reference prevents release of the transaction.

Fix this by only dropping the last reference to a transaction
in gsi_trans_free() while holding the spinlock.

Fixes: 9dd441e4ed575 ("soc: qcom: ipa: GSI transactions")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi_trans.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index 92642030e7356..e8599bb948c08 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -362,22 +362,31 @@ struct gsi_trans *gsi_channel_trans_alloc(struct gsi *gsi, u32 channel_id,
 	return trans;
 }
 
-/* Free a previously-allocated transaction (used only in case of error) */
+/* Free a previously-allocated transaction */
 void gsi_trans_free(struct gsi_trans *trans)
 {
+	refcount_t *refcount = &trans->refcount;
 	struct gsi_trans_info *trans_info;
+	bool last;
 
-	if (!refcount_dec_and_test(&trans->refcount))
+	/* We must hold the lock to release the last reference */
+	if (refcount_dec_not_one(refcount))
 		return;
 
 	trans_info = &trans->gsi->channel[trans->channel_id].trans_info;
 
 	spin_lock_bh(&trans_info->spinlock);
 
-	list_del(&trans->links);
+	/* Reference might have been added before we got the lock */
+	last = refcount_dec_and_test(refcount);
+	if (last)
+		list_del(&trans->links);
 
 	spin_unlock_bh(&trans_info->spinlock);
 
+	if (!last)
+		return;
+
 	ipa_gsi_trans_release(trans);
 
 	/* Releasing the reserved TREs implicitly frees the sgl[] and
-- 
2.20.1

Re: [PATCH net] net: ipa: lock when freeing transaction

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sat, 14 Nov 2020 12:20:17 -0600 you wrote:
> Transactions sit on one of several lists, depending on their state
> (allocated, pending, complete, or polled).  A spinlock protects
> against concurrent access when transactions are moved between these
> lists.
> 
> Transactions are also reference counted.  A newly-allocated
> transaction has an initial count of 1; a transaction is released in
> gsi_trans_free() only if its decremented reference count reaches 0.
> Releasing a transaction includes removing it from the polled (or if
> unused, allocated) list, so the spinlock is acquired when we release
> a transaction.
> 
> [...]

Here is the summary with links:
  - [net] net: ipa: lock when freeing transaction
    https://git.kernel.org/netdev/net/c/064c9c32b17c

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html