| Message ID | 20240116032732.65262-1-chentao@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/adreno: Add a null pointer check to the zap_shader_load_mdt | expand |
> kasprintf() returns a pointer to dynamically allocated memory > which can be NULL upon failure. Ensure the allocation was successful > by checking the pointer validity. … > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -144,6 +144,10 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, > char *newname; > > newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); > + if (!newname) { > + ret = -ENOMEM; > + goto out; > + } … How do you think about to avoid the repetition of the pointer check for the variable “mem_region”? Can the usage of other labels become more appropriate? Regards, Markus
On 2024/1/18 02:50, Markus Elfring wrote: >> kasprintf() returns a pointer to dynamically allocated memory >> which can be NULL upon failure. Ensure the allocation was successful >> by checking the pointer validity. > … >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> @@ -144,6 +144,10 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, >> char *newname; >> >> newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); >> + if (!newname) { >> + ret = -ENOMEM; >> + goto out; >> + } > … > > How do you think about to avoid the repetition of the pointer check > for the variable “mem_region”? "mem_region"? Is this a clerical error, do you mean 'newname'? No check found in __qcom_mdt_load for 'newname'. 'newname' is used for printing in '__qcom_mdt_load' in some cases, which is a bit dangerous. So it's necessary check it before using it. > Can the usage of other labels become more appropriate? > > Regards, > Markus
>>> kasprintf() returns a pointer to dynamically allocated memory >>> which can be NULL upon failure. Ensure the allocation was successful >>> by checking the pointer validity. >> … >>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>> @@ -144,6 +144,10 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, >>> char *newname; >>> >>> newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); >>> + if (!newname) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >> … >> >> How do you think about to avoid the repetition of the pointer check >> for the variable “mem_region”? > "mem_region"? Is this a clerical error, do you mean 'newname'? Please take another look at implementation details: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/msm/adreno/adreno_gpu.c?h=v6.7#n124 … >> Can the usage of other labels become more appropriate? I propose to reconsider also the influence of the label “out” here. https://elixir.bootlin.com/linux/v6.7/source/drivers/gpu/drm/msm/adreno/adreno_gpu.c#L167 Regards, Markus
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 074fb498706f..7e79ead4fe00 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -144,6 +144,10 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, char *newname; newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname); + if (!newname) { + ret = -ENOMEM; + goto out; + } ret = qcom_mdt_load(dev, fw, newname, pasid, mem_region, mem_phys, mem_size, NULL);
kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. Ensure the allocation was successful by checking the pointer validity. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++ 1 file changed, 4 insertions(+)