Re: [PATCH 10/16] drm/amdgpu: validate doorbell read/write

[Shashank Sharma <shashank.sharma@xxxxxxx>]

On 30/03/2023 16:49, Luben Tuikov wrote:
> On 2023-03-30 10:37, Shashank Sharma wrote:
> > On 30/03/2023 16:34, Luben Tuikov wrote:
> > > On 2023-03-29 11:47, Shashank Sharma wrote:
> > > > This patch:
> > > > - updates start/end values for each of the doorbell object
> > > >     created.
> > > > - adds a function which validates that the kernel doorbell read/write
> > > >     is within this range.
> > > > - uses this function during doorbell writes from kernel.
> > > >
> > > > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > Cc: Christian Koenig <christian.koenig@xxxxxxx>
> > > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
> > > > Signed-off-by: Arvind Yadav <arvind.yadav@xxxxxxx>
> > > > ---
> > > >    .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c  | 29 ++++++++++++++++---
> > > >    1 file changed, 25 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
> > > > index b46fe8b1378d..81713b2c28e1 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
> > > > @@ -22,6 +22,25 @@
> > > >     */
> > > >    
> > > >    #include "amdgpu.h"
> > > > +#include "kfd_priv.h"
> > > > +
> > > > +static inline
> > > > +bool amdgpu_doorbell_valid(struct amdgpu_device *adev, u32 index)
> > > > +{
> > > > +	if (index >= adev->doorbell.kernel_doorbells.start &&
> > > > +	    index < adev->doorbell.kernel_doorbells.end)
> > > > +		return true;
> > > > +
> > > > +	if (index >= adev->mes.kernel_doorbells.start &&
> > > > +	    index < adev->mes.kernel_doorbells.end)
> > > > +		return true;
> > > > +
> > > > +	if (index >= adev->kfd.dev->kernel_doorbells.start &&
> > > > +	    index < adev->kfd.dev->kernel_doorbells.end)
> > > > +		return true;
> > > > +
> > > > +	return false;
> > > > +}
> > > Here you're excluding "end".
> > >
> > > In patch 7 we see this:
> > > > +	/* Last index in this object */
> > > > +	uint32_t end;
> > > Which implies that "end" is included, but in this patch, the code excludes it.
> > > Perhaps you intended to use "index <= ...end" here?
> > No, this is intended, same as array object calculation.
> >
> > end = start + size;
> >
> > max = start + size - 1
> This I understand, but "end" is NEVER "start + size" in all
> code written since 1969. "end" is outside the bounds and thus
> never used like that.
> "start" and "end" usage comes from RTL and is always inclusive,
> and "end" always fits in the same sized register as that of "start".
> But if you use "size" and add, it may overflow. So, enough history.
>
> "end" is inclusive. If this is not the case in your implementation,
> then please use "size".
>
> > so (< end) not (<= end)
> >
> > end says last index in this doorbell range;
> This I don't understand.
>
> This isn't how "start" and "end" are being used.
> Their usage comes from RTL, and is always inclusive.
>
> Either use "start" and "size" or make "end" be inclusive.
>
> I'd prefer using "start" and "size" as this is traditionally
> what is done in memory management in software (not RTL).
>
> However using "end" in software makes it tricky to calculate
> size, and one always does "end-start+1", and this could lead
> to bugs and errors.
>
> Please use "start" and "size", then.

I think I am getting your point here, it should be end inclusive in this 
case, noted.

- Shashank


> Regards,
> Luben
>
>
> > - Shashank
> >
> > > Since this isn't RTL, wouldn't it be better to describe the doorbell instance,
> > > with a "start" and "size"? This is traditionally used in memory management,
> > > and it makes comparisons and checks easy.
> > >
> > > Regards,
> > > Luben
> > >
> > >
> > > >    
> > > >    /**
> > > >     * amdgpu_mm_rdoorbell - read a doorbell dword
> > > > @@ -37,7 +56,7 @@ u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32 index)
> > > >    	if (amdgpu_device_skip_hw_access(adev))
> > > >    		return 0;
> > > >    
> > > > -	if (index < adev->doorbell.num_kernel_doorbells) {
> > > > +	if (amdgpu_doorbell_valid(adev, index)) {
> > > >    		return readl(adev->doorbell.ptr + index);
> > > >    	} else {
> > > >    		DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", index);
> > > > @@ -60,7 +79,7 @@ void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32 index, u32 v)
> > > >    	if (amdgpu_device_skip_hw_access(adev))
> > > >    		return;
> > > >    
> > > > -	if (index < adev->doorbell.num_kernel_doorbells) {
> > > > +	if (amdgpu_doorbell_valid(adev, index)) {
> > > >    		writel(v, adev->doorbell.ptr + index);
> > > >    	} else {
> > > >    		DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index);
> > > > @@ -81,7 +100,7 @@ u64 amdgpu_mm_rdoorbell64(struct amdgpu_device *adev, u32 index)
> > > >    	if (amdgpu_device_skip_hw_access(adev))
> > > >    		return 0;
> > > >    
> > > > -	if (index < adev->doorbell.num_kernel_doorbells) {
> > > > +	if (amdgpu_doorbell_valid(adev, index)) {
> > > >    		return atomic64_read((atomic64_t *)(adev->doorbell.ptr + index));
> > > >    	} else {
> > > >    		DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", index);
> > > > @@ -104,7 +123,7 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
> > > >    	if (amdgpu_device_skip_hw_access(adev))
> > > >    		return;
> > > >    
> > > > -	if (index < adev->doorbell.num_kernel_doorbells) {
> > > > +	if (amdgpu_doorbell_valid(adev, index)) {
> > > >    		atomic64_set((atomic64_t *)(adev->doorbell.ptr + index), v);
> > > >    	} else {
> > > >    		DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index);
> > > > @@ -157,6 +176,8 @@ int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
> > > >    		return r;
> > > >    	}
> > > >    
> > > > +	db_obj->start = amdgpu_doorbell_index_on_bar(adev, db_obj->bo, 0);
> > > > +	db_obj->end = db_obj->start + db_obj->size / sizeof(u32);
> > > >    	return 0;
> > > >    }
> > > >