An improper locking bug(e.g., deadlock) on the lock up_inode_ctx->client_list_lock

[ryancaicse]

Hi, developers, thank you for your checking. It seems the lock up_inode_ctx->client_list_lock is not released correctly when __upcall_cleanup_client_entry(up_client); returns true in the function upcall_cleanup_expired_clients?

glusterfs/xlators/features/upcall/src/upcall-internal.c

Lines 198 to 225 in 3503925

	pthread_mutex_lock(&up_inode_ctx->client_list_lock);
	{
	list_for_each_entry_safe(up_client, tmp, &up_inode_ctx->client_list,
	client_list)
	{
	t_expired = now - up_client->access_time;
	if (t_expired > (2 * timeout)) {
	gf_log(THIS->name, GF_LOG_TRACE, "Cleaning up client_entry(%s)",
	up_client->client_uid);
	ret = __upcall_cleanup_client_entry(up_client);
	if (ret) {
	gf_msg("upcall", GF_LOG_WARNING, 0,
	UPCALL_MSG_INTERNAL_ERROR,
	"Client entry cleanup failed (%p)", up_client);
	goto out;
	}
	}
	}
	}
	pthread_mutex_unlock(&up_inode_ctx->client_list_lock);
	ret = 0;
	out:
	return ret;
	}

Best,

[ryancaicse]

Similarly, for the lock pl_inode->mutex

glusterfs/xlators/features/locks/src/common.c

Lines 1397 to 1416 in ecc99cc

	pthread_mutex_lock(&pl_inode->mutex);
	if (pl_inode_has_owners(xl, frame->root->client, pl_inode, &now, contend)) {
	error = -1;
	/* We skip the unlock here because the caller must create a stub when
	* we return -1 and do a call to pl_inode_remove_complete(), which
	* assumes the lock is still acquired and will release it once
	* everything else is prepared. */
	goto done;
	}
	pl_inode->is_locked = _gf_true;
	pl_inode->remove_running++;
	pthread_mutex_unlock(&pl_inode->mutex);
	done:
	*ppl_inode = pl_inode;
	return error;

[pranithk]

@ryancaicse First one is a bug. 2nd one has a comment that explains that the unlock should be taken care by the caller and it seems to be doing so. Do you want to send a PR for the first one?

[ryancaicse]

@pranithk Thanks

[ryancaicse]

Hi, any security impacts for the first bug?
I think there could be security issues - unreleased lock could lead to deadlock(a DoS), wreaking a DoS.
Could we apply CVE for this?

[mykaul]

Hi, any security impacts for the first bug? I think there could be security issues - unreleased lock could lead to deadlock(a DoS), wreaking a DoS. Could we apply CVE for this?

@ryancaicse - how could __upcall_cleanup_client_entry() return anything but 0?

[ryancaicse]

Hi, any security impacts for the first bug? I think there could be security issues - unreleased lock could lead to deadlock(a DoS), wreaking a DoS. Could we apply CVE for this?

@ryancaicse - how could __upcall_cleanup_client_entry() return anything but 0?

I don't know since I did not write them. I am also asking questions.

[pranithk]

As it stands now, it won't. the patch is more of future proofing if someone were to introduce cases where -ve value will be returned. So it is not a CVE

[ryancaicse]

@pranithk @mykaul Many thanks for your reply

[stale]

Thank you for your contributions.
Noticed that this issue is not having any activity in last ~6 months! We are marking this issue as stale because it has not had recent activity.
It will be closed in 2 weeks if no one responds with a comment here.

[stale]

Closing this issue as there was no update since my last update on issue. If this is an issue which is still valid, feel free to open it.