[Skiboot] [PATCH] fsp-elog: Add various NULL checks

[Vasant Hegde]

On 12/09/2014 08:15 AM, Benjamin Herrenschmidt wrote:
> Recent gcc 4.9 will silently add conditional traps for NULL checks in
> cases where it thinks we may dereference a NULL pointer. It seems to
> be pretty keen on doing so when we dereference the result of list_top.
>
> Most of the time, these aren't bugs because we have some other state
> variable that tells us whether our list contains something or not but
> we hit a bug in the FSP code recently where that was getting out of
> sync, and the result of a trap in real mode isn't pretty ....
>
> So this adds explicit NULL checks in a number of place where gcc added
> trap instructions. With this patch, the current tree doesn't generate
> any. I didn't find a way to make gcc warn unfortunately.
>
> Signed-off-by: Benjamin Herrenschmidt <benh at kernel.crashing.org>


Reviewed-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>

-Vasant

> ---
>
> diff --git a/hw/fsp/fsp-elog-read.c b/hw/fsp/fsp-elog-read.c
> index ede958c..435612e 100644
> --- a/hw/fsp/fsp-elog-read.c
> +++ b/hw/fsp/fsp-elog-read.c
> @@ -173,10 +173,16 @@ static void fsp_elog_fetch_failure(uint8_t fsp_status)
>
>   	/* read top list and delete the node */
>   	log_data = list_top(&elog_read_pending, struct fsp_log_entry, link);
> -	list_del(&log_data->link);
> -	list_add(&elog_read_free, &log_data->link);
> -	prerror("ELOG: received invalid data: %x FSP status: 0x%x\n",
> -		log_data->log_id, fsp_status);
> +	if (!log_data) {
> +		prlog(PR_ERR, "%s: Inconsistent internal list state !\n",
> +		      __func__);
> +	} else {
> +		list_del(&log_data->link);
> +		list_add(&elog_read_free, &log_data->link);
> +		prerror("ELOG: received invalid data: %x FSP status: 0x%x\n",
> +			log_data->log_id, fsp_status);
> +
> +	}
>   	fsp_elog_set_head_state(ELOG_STATE_NONE);
>   }
>
> @@ -228,6 +234,12 @@ static void fsp_elog_queue_fetch(void)
>   	struct fsp_log_entry *entry;
>
>   	entry = list_top(&elog_read_pending, struct fsp_log_entry, link);
> +	if (!entry) {
> +		prlog(PR_ERR, "%s: Inconsistent internal list state !\n",
> +		      __func__);
> +		fsp_elog_set_head_state(ELOG_STATE_NONE);
> +		return;
> +	}
>   	fsp_elog_set_head_state(ELOG_STATE_FETCHING);
>   	elog_head_id = entry->log_id;
>   	elog_head_size = entry->log_size;
> @@ -260,6 +272,12 @@ static int64_t fsp_opal_elog_info(uint64_t *opal_elog_id,
>   		return OPAL_WRONG_STATE;
>   	}
>   	log_data = list_top(&elog_read_pending, struct fsp_log_entry, link);
> +	if (!log_data) {
> +		prlog(PR_ERR, "%s: Inconsistent internal list state !\n",
> +		      __func__);
> +		unlock(&elog_read_lock);
> +		return OPAL_WRONG_STATE;
> +	}
>   	*opal_elog_id = log_data->log_id;
>   	*opal_elog_size = log_data->log_size;
>   	unlock(&elog_read_lock);
> @@ -288,6 +306,12 @@ static int64_t fsp_opal_elog_read(uint64_t *buffer, uint64_t opal_elog_size,
>   	}
>
>   	log_data = list_top(&elog_read_pending, struct fsp_log_entry, link);
> +	if (!log_data) {
> +		prlog(PR_ERR, "%s: Inconsistent internal list state !\n",
> +		      __func__);
> +		unlock(&elog_read_lock);
> +		return OPAL_WRONG_STATE;
> +	}
>
>   	/* Check log ID and then read log from buffer */
>   	if (opal_elog_id != log_data->log_id) {
> diff --git a/hw/fsp/fsp-elog-write.c b/hw/fsp/fsp-elog-write.c
> index 3a9adda..dd71712 100644
> --- a/hw/fsp/fsp-elog-write.c
> +++ b/hw/fsp/fsp-elog-write.c
> @@ -134,10 +134,17 @@ bool opal_elog_info(uint64_t *opal_elog_id, uint64_t *opal_elog_size)
>   	if (elog_write_to_host_head_state == ELOG_STATE_FETCHED_DATA) {
>   		head = list_top(&elog_write_to_host_pending,
>   					struct errorlog, link);
> -		*opal_elog_id = head->plid;
> -		*opal_elog_size = head->log_size;
> -		elog_write_to_host_head_state = ELOG_STATE_FETCHED_INFO;
> -		rc = true;
> +		if (!head) {
> +			prlog(PR_ERR,
> +			      "%s: Inconsistent internal list state !\n",
> +			      __func__);
> +			elog_write_to_host_head_state = ELOG_STATE_NONE;
> +		} else {
> +			*opal_elog_id = head->plid;
> +			*opal_elog_size = head->log_size;
> +			elog_write_to_host_head_state = ELOG_STATE_FETCHED_INFO;
> +			rc = true;
> +		}
>   	}
>   	unlock(&elog_write_to_host_lock);
>   	return rc;
> @@ -165,7 +172,7 @@ static void opal_commit_elog_in_host(void)
>
>
>   bool opal_elog_read(uint64_t *buffer, uint64_t opal_elog_size,
> -						uint64_t opal_elog_id)
> +		    uint64_t opal_elog_id)
>   {
>   	struct errorlog *log_data;
>   	bool rc = false;
> @@ -174,9 +181,13 @@ bool opal_elog_read(uint64_t *buffer, uint64_t opal_elog_size,
>   	if (elog_write_to_host_head_state == ELOG_STATE_FETCHED_INFO) {
>   		log_data = list_top(&elog_write_to_host_pending,
>   					struct errorlog, link);
> -
> +		if (!log_data) {
> +			elog_write_to_host_head_state = ELOG_STATE_NONE;
> +			unlock(&elog_write_to_host_lock);
> +			return rc;
> +		}
>   		if ((opal_elog_id != log_data->plid) &&
> -				(opal_elog_size != log_data->log_size)) {
> +		    (opal_elog_size != log_data->log_size)) {
>   			unlock(&elog_write_to_host_lock);
>   			return rc;
>   		}
>
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>