Re: [PATCH v6 1/3] soc: qcom: Introduce Protection Domain Restart helpers

[Bjorn Andersson]

On Wed 04 Mar 12:09 PST 2020, Sibi Sankar wrote:
> diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c
[..]
> +static int pdr_locate_service(struct pdr_handle *pdr, struct pdr_service *pds)
> +{
> +	struct servreg_get_domain_list_resp *resp;
> +	struct servreg_get_domain_list_req req;
> +	struct servreg_location_entry *entry;
> +	int domains_read = 0;
> +	int ret, i;
> +
> +	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
> +	if (!resp)
> +		return -ENOMEM;
> +
> +	/* Prepare req message */
> +	strcpy(req.service_name, pds->service_name);
> +	req.domain_offset_valid = true;
> +	req.domain_offset = 0;
> +
> +	do {
> +		req.domain_offset = domains_read;
> +		ret = pdr_get_domain_list(&req, resp, pdr);
> +		if (ret < 0)
> +			goto out;
> +
> +		for (i = domains_read; i < resp->domain_list_len; i++) {
> +			entry = &resp->domain_list[i];
> +
> +			if (strnlen(entry->name, sizeof(entry->name)) == sizeof(entry->name))
> +				continue;
> +
> +			if (!strcmp(entry->name, pds->service_path)) {
> +				pds->service_data_valid = entry->service_data_valid;
> +				pds->service_data = entry->service_data;
> +				pds->instance = entry->instance;
> +				goto out;
> +			}
> +		}
> +
> +		/* Update ret to indicate that the service is not yet found */
> +		ret = -ENXIO;
> +
> +		/* Always read total_domains from the response msg */
> +		if (resp->domain_list_len > resp->total_domains)
> +			resp->domain_list_len = resp->total_domains;
> +
> +		domains_read += resp->domain_list_len;
> +	} while (domains_read < resp->total_domains);
> +out:
> +	kfree(resp);
> +	return ret;
> +}
> +
> +static void pdr_notify_lookup_failure(struct pdr_handle *pdr,
> +				      struct pdr_service *pds,
> +				      int err)
> +{
> +	list_del(&pds->node);
> +
> +	if (err == -ENXIO)
> +		pds->state = SERVREG_LOCATOR_UNKNOWN_SERVICE;
> +	else
> +		pds->state = SERVREG_LOCATOR_ERR;
> +
> +	pr_err("PDR: service lookup for %s failed: %d\n",
> +	       pds->service_name, err);
> +
> +	mutex_lock(&pdr->status_lock);
> +	pdr->status(pds->state, pds->service_path, pdr->priv);
> +	mutex_unlock(&pdr->status_lock);
> +	kfree(pds);

So this implies that we didn't find the service and we will never find
it? How are the client drivers expected to react to above two states?

> +}
> +
> +static void pdr_locator_work(struct work_struct *work)
> +{
> +	struct pdr_handle *pdr = container_of(work, struct pdr_handle,
> +					      locator_work);
> +	struct pdr_service *pds, *tmp;
> +	int ret = 0;
> +
> +	/* Bail out early if the SERVREG LOCATOR QMI service is not up */
> +	mutex_lock(&pdr->lock);
> +	if (!pdr->locator_init_complete) {
> +		mutex_unlock(&pdr->lock);
> +		pr_debug("PDR: SERVICE LOCATOR service not available\n");
> +		return;
> +	}
> +	mutex_unlock(&pdr->lock);
> +
> +	mutex_lock(&pdr->list_lock);
> +	list_for_each_entry_safe(pds, tmp, &pdr->lookups, node) {
> +		if (!pds->need_locator_lookup)
> +			continue;
> +
> +		pds->need_locator_lookup = false;
> +		ret = pdr_locate_service(pdr, pds);
> +		if (ret < 0)
> +			pdr_notify_lookup_failure(pdr, pds, ret);

If I read this correctly, pdr_locate_service() returning an error seems
to mean that pd->instance won't be filled out, as such I don't think you
want to proceed.

Further more, pdr_notify_lookup_failure() ends up freeing the pds, so
below lookup would be a use after free, not unlikely followed by a
double free of pds.

How about a "continue" here and only clear need_locator_lookup if both
of these checks pass?

> +
> +		ret = qmi_add_lookup(&pdr->notifier_hdl, pds->service, 1,
> +				     pds->instance);
> +		if (ret < 0)
> +			pdr_notify_lookup_failure(pdr, pds, ret);
> +	}
> +	mutex_unlock(&pdr->list_lock);
> +}

Apart from that I think the patches look good now.

Regards,
Bjorn