[PATCH] drm/i915/gt: handle null ptr at sg traversing

[PATCH] drm/i915/gt: handle null ptr at sg traversing

[Ramalingam C]

When calculating the starting address for ccs data in smem scatterlist,
handle the NULL pointer returned from sg_next, incase of scatterlist
less than required size..

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_migrate.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c
index 2c35324b5f68..c206fb4f4186 100644
--- a/drivers/gpu/drm/i915/gt/intel_migrate.c
+++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
@@ -669,7 +669,7 @@ calculate_chunk_sz(struct drm_i915_private *i915, bool src_is_lmem,
 	}
 }
 
-static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
+static int get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
 {
 	u32 len;
 
@@ -684,9 +684,13 @@ static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
 		bytes_to_cpy -= len;
 
 		it->sg = __sg_next(it->sg);
+		if (!it->sg)
+			return -EINVAL;
 		it->dma = sg_dma_address(it->sg);
 		it->max = it->dma + sg_dma_len(it->sg);
 	} while (bytes_to_cpy);
+
+	return 0;
 }
 
 int
@@ -745,8 +749,11 @@ intel_context_migrate_copy(struct intel_context *ce,
 		 * Need to fix it.
 		 */
 		ccs_bytes_to_cpy = src_sz != dst_sz ? GET_CCS_BYTES(i915, bytes_to_cpy) : 0;
-		if (ccs_bytes_to_cpy)
-			get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
+		if (ccs_bytes_to_cpy) {
+			err = get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
+			if (err)
+				return err;
+		}
 	}
 
 	src_offset = 0;
-- 
2.20.1

Re: [PATCH] drm/i915/gt: handle null ptr at sg traversing

[Matthew Auld]

On 27/06/2022 18:35, Ramalingam C wrote:
> When calculating the starting address for ccs data in smem scatterlist,
> handle the NULL pointer returned from sg_next, incase of scatterlist
> less than required size..

Do we have some more information on how we can hit this? Is this a 
programmer error? Do we have a testcase?

> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_migrate.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c
> index 2c35324b5f68..c206fb4f4186 100644
> --- a/drivers/gpu/drm/i915/gt/intel_migrate.c
> +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
> @@ -669,7 +669,7 @@ calculate_chunk_sz(struct drm_i915_private *i915, bool src_is_lmem,
>   	}
>   }
>   
> -static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
> +static int get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
>   {
>   	u32 len;
>   
> @@ -684,9 +684,13 @@ static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
>   		bytes_to_cpy -= len;
>   
>   		it->sg = __sg_next(it->sg);
> +		if (!it->sg)
> +			return -EINVAL;
>   		it->dma = sg_dma_address(it->sg);
>   		it->max = it->dma + sg_dma_len(it->sg);
>   	} while (bytes_to_cpy);
> +
> +	return 0;
>   }
>   
>   int
> @@ -745,8 +749,11 @@ intel_context_migrate_copy(struct intel_context *ce,
>   		 * Need to fix it.
>   		 */
>   		ccs_bytes_to_cpy = src_sz != dst_sz ? GET_CCS_BYTES(i915, bytes_to_cpy) : 0;
> -		if (ccs_bytes_to_cpy)
> -			get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
> +		if (ccs_bytes_to_cpy) {
> +			err = get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
> +			if (err)
> +				return err;
> +		}
>   	}
>   
>   	src_offset = 0;

Re: [PATCH] drm/i915/gt: handle null ptr at sg traversing

[Ramalingam C]

On 2022-06-28 at 10:40:56 +0100, Matthew Auld wrote:
> On 27/06/2022 18:35, Ramalingam C wrote:
> > When calculating the starting address for ccs data in smem scatterlist,
> > handle the NULL pointer returned from sg_next, incase of scatterlist
> > less than required size..
> 
> Do we have some more information on how we can hit this? Is this a
> programmer error? Do we have a testcase?
Typically We will never get NULL at this point, as we allocate the smem
of sz equal to lmem obj size + requiured ccs size. So we will never run
into NULL when we traverse the sg for the size of lmem in smem's sg.

IF there is NULL returned in this scenario we could report BUG_ON or let
it NPD or return the error code.

But either way couldn't think of a scenario when this will hit. after
thinking further seems to be leaving the NPD itself sufficient as other
error handling also not doing good job at it. Please share your thoughts

Ram
> 
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_migrate.c | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c
> > index 2c35324b5f68..c206fb4f4186 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_migrate.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
> > @@ -669,7 +669,7 @@ calculate_chunk_sz(struct drm_i915_private *i915, bool src_is_lmem,
> >   	}
> >   }
> > -static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
> > +static int get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
> >   {
> >   	u32 len;
> > @@ -684,9 +684,13 @@ static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
> >   		bytes_to_cpy -= len;
> >   		it->sg = __sg_next(it->sg);
> > +		if (!it->sg)
> > +			return -EINVAL;
> >   		it->dma = sg_dma_address(it->sg);
> >   		it->max = it->dma + sg_dma_len(it->sg);
> >   	} while (bytes_to_cpy);
> > +
> > +	return 0;
> >   }
> >   int
> > @@ -745,8 +749,11 @@ intel_context_migrate_copy(struct intel_context *ce,
> >   		 * Need to fix it.
> >   		 */
> >   		ccs_bytes_to_cpy = src_sz != dst_sz ? GET_CCS_BYTES(i915, bytes_to_cpy) : 0;
> > -		if (ccs_bytes_to_cpy)
> > -			get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
> > +		if (ccs_bytes_to_cpy) {
> > +			err = get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
> > +			if (err)
> > +				return err;
> > +		}
> >   	}
> >   	src_offset = 0;