| Message ID | 10fd258b-466f-4c5b-9d48-fe61a3f21424@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nouveau/u_memcpya: fix NULL vs error pointer bug | expand |
Hi Dan, On 9/15/23 14:59, Dan Carpenter wrote: > The u_memcpya() function is supposed to return error pointers on > error. Returning NULL will lead to an Oops. > > Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h > index 3666a7403e47..52a708a98915 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h > @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) > size_t bytes; > > if (unlikely(check_mul_overflow(nmemb, size, &bytes))) > - return NULL; > + return ERR_PTR(-ENOMEM); I plan to replace this function with an upcoming vmemdup_array_user() helper, which returns -EOVERFLOW instead, hence mind using that? Unless you disagree, no need to resubmit the patch, I can change it before applying the patch. - Danilo > return vmemdup_user(userptr, bytes); > } >
On Sat, Sep 16, 2023 at 01:41:43AM +0200, Danilo Krummrich wrote: > Hi Dan, > > On 9/15/23 14:59, Dan Carpenter wrote: > > The u_memcpya() function is supposed to return error pointers on > > error. Returning NULL will lead to an Oops. > > > > Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h > > index 3666a7403e47..52a708a98915 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h > > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h > > @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) > > size_t bytes; > > if (unlikely(check_mul_overflow(nmemb, size, &bytes))) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > > I plan to replace this function with an upcoming vmemdup_array_user() helper, > which returns -EOVERFLOW instead, hence mind using that? > > Unless you disagree, no need to resubmit the patch, I can change it > before applying the patch. Generally, I would say that ENOMEM is the correct error code. I feel like someone thinks EOVERFLOW means integer overflow and that's not correct. I means like if you pass a number higher than INT_MAX to kstroint(). But I don't care strongly about this. You can change it if you want to. regards, dan carpenter
On Sat, Sep 16, 2023 at 05:24:04PM +0300, Dan Carpenter wrote: > On Sat, Sep 16, 2023 at 01:41:43AM +0200, Danilo Krummrich wrote: > > Hi Dan, > > > > On 9/15/23 14:59, Dan Carpenter wrote: > > > The u_memcpya() function is supposed to return error pointers on > > > error. Returning NULL will lead to an Oops. > > > > > > Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h > > > index 3666a7403e47..52a708a98915 100644 > > > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h > > > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h > > > @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) > > > size_t bytes; > > > if (unlikely(check_mul_overflow(nmemb, size, &bytes))) > > > - return NULL; > > > + return ERR_PTR(-ENOMEM); > > > > I plan to replace this function with an upcoming vmemdup_array_user() helper, > > which returns -EOVERFLOW instead, hence mind using that? > > > > Unless you disagree, no need to resubmit the patch, I can change it > > before applying the patch. > > Generally, I would say that ENOMEM is the correct error code. I feel > like someone thinks EOVERFLOW means integer overflow and that's not > correct. I means like if you pass a number higher than INT_MAX to > kstroint(). The most common error code for integer overflows is EINVAL because the user passed invalid data. regards, dan carpenter
On 9/16/23 16:26, Dan Carpenter wrote: > On Sat, Sep 16, 2023 at 05:24:04PM +0300, Dan Carpenter wrote: >> On Sat, Sep 16, 2023 at 01:41:43AM +0200, Danilo Krummrich wrote: >>> Hi Dan, >>> >>> On 9/15/23 14:59, Dan Carpenter wrote: >>>> The u_memcpya() function is supposed to return error pointers on >>>> error. Returning NULL will lead to an Oops. >>>> >>>> Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") >>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >>>> --- >>>> drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h >>>> index 3666a7403e47..52a708a98915 100644 >>>> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h >>>> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h >>>> @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) >>>> size_t bytes; >>>> if (unlikely(check_mul_overflow(nmemb, size, &bytes))) >>>> - return NULL; >>>> + return ERR_PTR(-ENOMEM); >>> >>> I plan to replace this function with an upcoming vmemdup_array_user() helper, >>> which returns -EOVERFLOW instead, hence mind using that? >>> >>> Unless you disagree, no need to resubmit the patch, I can change it >>> before applying the patch. >> >> Generally, I would say that ENOMEM is the correct error code. I feel >> like someone thinks EOVERFLOW means integer overflow and that's not >> correct. I means like if you pass a number higher than INT_MAX to >> kstroint(). > > The most common error code for integer overflows is EINVAL because the > user passed invalid data. I totally agree with that, and my choice would have been EINVAL as well. It's just that it seems (v)memdup_array_user() [1] goes with that and hence I'd just go along. [1] https://lore.kernel.org/lkml/93001a9f3f101be0f374080090f9c32df73ca773.1694202430.git.pstanner@redhat.com/ > > regards, > dan carpenter >
Reviewed-by: Lyude Paul <lyude@redhat.com> I assume you need me to push this to drm-misc? On Fri, 2023-09-15 at 15:59 +0300, Dan Carpenter wrote: > The u_memcpya() function is supposed to return error pointers on > error. Returning NULL will lead to an Oops. > > Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h > index 3666a7403e47..52a708a98915 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h > @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) > size_t bytes; > > if (unlikely(check_mul_overflow(nmemb, size, &bytes))) > - return NULL; > + return ERR_PTR(-ENOMEM); > return vmemdup_user(userptr, bytes); > } >
On 9/16/23 16:24, Dan Carpenter wrote: > On Sat, Sep 16, 2023 at 01:41:43AM +0200, Danilo Krummrich wrote: >> Hi Dan, >> >> On 9/15/23 14:59, Dan Carpenter wrote: >>> The u_memcpya() function is supposed to return error pointers on >>> error. Returning NULL will lead to an Oops. >>> >>> Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") >>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >>> --- >>> drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h >>> index 3666a7403e47..52a708a98915 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h >>> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h >>> @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) >>> size_t bytes; >>> if (unlikely(check_mul_overflow(nmemb, size, &bytes))) >>> - return NULL; >>> + return ERR_PTR(-ENOMEM); >> >> I plan to replace this function with an upcoming vmemdup_array_user() helper, >> which returns -EOVERFLOW instead, hence mind using that? >> >> Unless you disagree, no need to resubmit the patch, I can change it >> before applying the patch. > > Generally, I would say that ENOMEM is the correct error code. I feel > like someone thinks EOVERFLOW means integer overflow and that's not > correct. I means like if you pass a number higher than INT_MAX to > kstroint(). > > But I don't care strongly about this. You can change it if you want to. I seems that vmemdup_array_user() will keep using EOVERFLOW, hence aligning to that. Pushed the patch to drm-misc-fixes, thanks! > > regards, > dan carpenter >
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h index 3666a7403e47..52a708a98915 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h @@ -193,7 +193,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size) size_t bytes; if (unlikely(check_mul_overflow(nmemb, size, &bytes))) - return NULL; + return ERR_PTR(-ENOMEM); return vmemdup_user(userptr, bytes); }
The u_memcpya() function is supposed to return error pointers on error. Returning NULL will lead to an Oops. Fixes: 68132cc6d1bc ("nouveau/u_memcpya: use vmemdup_user") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)