Skip to contentSkip to conversation

drm/amdgpu/pm: Remove VLA usage

PATCH In review Kees Cook 3 participants 5 messages Jun 20, 2018 – Jul 18, 2018 3 messages not in the archive

Subsystems (2)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 drm/amdgpu/pm: Remove VLA usage

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum sane buffer size and removes copy/paste code.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++++++++++--------------
 1 file changed, 42 insertions(+), 58 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +42 -58
index b455da487782..5eb98cde22ed 100644
@@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
 		return snprintf(buf, PAGE_SIZE, "\n");
 }
 
-static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
-		struct device_attribute *attr,
-		const char *buf,
-		size_t count)
+/*
+ * Worst case: 32 bits individually specified, in octal at 12 characters
+ * per line (+1 for \n).
+ */
+#define AMDGPU_MASK_BUF_MAX	(32 * 13)
+
+static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t *mask)
 {
-	struct drm_device *ddev = dev_get_drvdata(dev);
-	struct amdgpu_device *adev = ddev->dev_private;
 	int ret;
 	long level;
-	uint32_t mask = 0;
 	char *sub_str = NULL;
 	char *tmp;
-	char buf_cpy[count];
+	char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
 	const char delimiter[3] = {' ', '\n', '\0'};
+	size_t bytes;
 
-	memcpy(buf_cpy, buf, count+1);
+	*mask = 0;
+
+	bytes = min(count, sizeof(buf_cpy) - 1);
+	memcpy(buf_cpy, buf, bytes);
+	buf_cpy[bytes] = '\0';
 	tmp = buf_cpy;
 	while (tmp[0]) {
-		sub_str =  strsep(&tmp, delimiter);
+		sub_str = strsep(&tmp, delimiter);
 		if (strlen(sub_str)) {
 			ret = kstrtol(sub_str, 0, &level);
-
-			if (ret) {
-				count = -EINVAL;
-				goto fail;
-			}
-			mask |= 1 << level;
+			if (ret)
+				return -EINVAL;
+			*mask |= 1 << level;
 		} else
 			break;
 	}
+
+	return 0;
+}
+
+static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf,
+		size_t count)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct amdgpu_device *adev = ddev->dev_private;
+	int ret;
+	uint32_t mask = 0;
+
+	ret = amdgpu_read_mask(buf, count, &mask);
+	if (ret)
+		return ret;
+
 	if (adev->powerplay.pp_funcs->force_clock_level)
 		amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
 
-fail:
 	return count;
 }
 
@@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device *dev,
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct amdgpu_device *adev = ddev->dev_private;
 	int ret;
-	long level;
 	uint32_t mask = 0;
-	char *sub_str = NULL;
-	char *tmp;
-	char buf_cpy[count];
-	const char delimiter[3] = {' ', '\n', '\0'};
 
-	memcpy(buf_cpy, buf, count+1);
-	tmp = buf_cpy;
-	while (tmp[0]) {
-		sub_str =  strsep(&tmp, delimiter);
-		if (strlen(sub_str)) {
-			ret = kstrtol(sub_str, 0, &level);
+	ret = amdgpu_read_mask(buf, count, &mask);
+	if (ret)
+		return ret;
 
-			if (ret) {
-				count = -EINVAL;
-				goto fail;
-			}
-			mask |= 1 << level;
-		} else
-			break;
-	}
 	if (adev->powerplay.pp_funcs->force_clock_level)
 		amdgpu_dpm_force_clock_level(adev, PP_MCLK, mask);
 
-fail:
 	return count;
 }
 
@@ -701,33 +703,15 @@ static ssize_t amdgpu_set_pp_dpm_pcie(struct device *dev,
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct amdgpu_device *adev = ddev->dev_private;
 	int ret;
-	long level;
 	uint32_t mask = 0;
-	char *sub_str = NULL;
-	char *tmp;
-	char buf_cpy[count];
-	const char delimiter[3] = {' ', '\n', '\0'};
-
-	memcpy(buf_cpy, buf, count+1);
-	tmp = buf_cpy;
 
-	while (tmp[0]) {
-		sub_str =  strsep(&tmp, delimiter);
-		if (strlen(sub_str)) {
-			ret = kstrtol(sub_str, 0, &level);
+	ret = amdgpu_read_mask(buf, count, &mask);
+	if (ret)
+		return ret;
 
-			if (ret) {
-				count = -EINVAL;
-				goto fail;
-			}
-			mask |= 1 << level;
-		} else
-			break;
-	}
 	if (adev->powerplay.pp_funcs->force_clock_level)
 		amdgpu_dpm_force_clock_level(adev, PP_PCIE, mask);
 
-fail:
 	return count;
 }
 
Signature · 6 lines
2.17.1


-- 
Kees Cook
Pixel Security
author
#1 lore raw
Quoted 6 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum sane buffer size and removes copy/paste code.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Friendly ping! Who's tree should this go through?

Thanks!

-Kees

Quoted 165 lines
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++++++++++--------------
 1 file changed, 42 insertions(+), 58 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +42 -59
index b455da487782..5eb98cde22ed 100644
@@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
                return snprintf(buf, PAGE_SIZE, "\n");
 }
 
-static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
-               struct device_attribute *attr,
-               const char *buf,
-               size_t count)
+/*
+ * Worst case: 32 bits individually specified, in octal at 12 characters
+ * per line (+1 for \n).
+ */
+#define AMDGPU_MASK_BUF_MAX    (32 * 13)
+
+static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t *mask)
 {
-       struct drm_device *ddev = dev_get_drvdata(dev);
-       struct amdgpu_device *adev = ddev->dev_private;
        int ret;
        long level;
-       uint32_t mask = 0;
        char *sub_str = NULL;
        char *tmp;
-       char buf_cpy[count];
+       char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
        const char delimiter[3] = {' ', '\n', '\0'};
+       size_t bytes;
 
-       memcpy(buf_cpy, buf, count+1);
+       *mask = 0;
+
+       bytes = min(count, sizeof(buf_cpy) - 1);
+       memcpy(buf_cpy, buf, bytes);
+       buf_cpy[bytes] = '\0';
        tmp = buf_cpy;
        while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
+               sub_str = strsep(&tmp, delimiter);
                if (strlen(sub_str)) {
                        ret = kstrtol(sub_str, 0, &level);
-
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
+                       if (ret)
+                               return -EINVAL;
+                       *mask |= 1 << level;
                } else
                        break;
        }
+
+       return 0;
+}
+
+static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
+               struct device_attribute *attr,
+               const char *buf,
+               size_t count)
+{
+       struct drm_device *ddev = dev_get_drvdata(dev);
+       struct amdgpu_device *adev = ddev->dev_private;
+       int ret;
+       uint32_t mask = 0;
+
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
+
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
 
-fail:
        return count;
 }
 
@@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device *dev,
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;
        int ret;
-       long level;
        uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
 
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_MCLK, mask);
 
-fail:
        return count;
 }
 
@@ -701,33 +703,15 @@ static ssize_t amdgpu_set_pp_dpm_pcie(struct device *dev,
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;
        int ret;
-       long level;
        uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
-
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
 
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_PCIE, mask);
 
-fail:
        return count;
 }
 
--

2.17.1

--
Kees Cook
Pixel Security

Signature · 2 lines
Kees Cook
Pixel Security
Reviewed-by Rex Zhu
#3 lore raw
In reply to a message not in the archive

Patch is:

Best Regards
Rex

________________________________
From: [email protected] <[email protected]> on behalf of Kees Cook <[email protected]>
Sent: Tuesday, July 17, 2018 11:59 AM
To: Deucher, Alexander
Cc: LKML; Koenig, Christian; Zhou, David(ChunMing); David Airlie; Zhu, Rex; Huang, Ray; Kuehling, Felix; amd-gfx list; Maling list - DRI developers
Subject: Re: [PATCH] drm/amdgpu/pm: Remove VLA usage

Quoted 6 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum sane buffer size and removes copy/paste code.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Friendly ping! Who's tree should this go through?

Thanks!

-Kees

Quoted 162 lines
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++++++++++--------------
 1 file changed, 42 insertions(+), 58 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +42 -59
index b455da487782..5eb98cde22ed 100644
@@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
                return snprintf(buf, PAGE_SIZE, "\n");
 }
 
-static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
-               struct device_attribute *attr,
-               const char *buf,
-               size_t count)
+/*
+ * Worst case: 32 bits individually specified, in octal at 12 characters
+ * per line (+1 for \n).
+ */
+#define AMDGPU_MASK_BUF_MAX    (32 * 13)
+
+static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t *mask)
 {
-       struct drm_device *ddev = dev_get_drvdata(dev);
-       struct amdgpu_device *adev = ddev->dev_private;
        int ret;
        long level;
-       uint32_t mask = 0;
        char *sub_str = NULL;
        char *tmp;
-       char buf_cpy[count];
+       char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
        const char delimiter[3] = {' ', '\n', '\0'};
+       size_t bytes;
 
-       memcpy(buf_cpy, buf, count+1);
+       *mask = 0;
+
+       bytes = min(count, sizeof(buf_cpy) - 1);
+       memcpy(buf_cpy, buf, bytes);
+       buf_cpy[bytes] = '\0';
        tmp = buf_cpy;
        while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
+               sub_str = strsep(&tmp, delimiter);
                if (strlen(sub_str)) {
                        ret = kstrtol(sub_str, 0, &level);
-
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
+                       if (ret)
+                               return -EINVAL;
+                       *mask |= 1 << level;
                } else
                        break;
        }
+
+       return 0;
+}
+
+static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
+               struct device_attribute *attr,
+               const char *buf,
+               size_t count)
+{
+       struct drm_device *ddev = dev_get_drvdata(dev);
+       struct amdgpu_device *adev = ddev->dev_private;
+       int ret;
+       uint32_t mask = 0;
+
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
+
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
 
-fail:
        return count;
 }
 
@@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device *dev,
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;
        int ret;
-       long level;
        uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
 
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_MCLK, mask);
 
-fail:
        return count;
 }
 
@@ -701,33 +703,15 @@ static ssize_t amdgpu_set_pp_dpm_pcie(struct device *dev,
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;
        int ret;
-       long level;
        uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
-
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
 
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_PCIE, mask);
 
-fail:
        return count;
 }
 
--

2.17.1

Quoted 3 lines

--
Kees Cook
Pixel Security

--
Kees Cook
Pixel Security

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

#5 lore raw
In reply to a message not in the archive
Quoted 1 line

Who's tree should this go through?

To answer the question: When Rex is ok with that he pushes it to our internal amd-staging-drm-next tree.

Alex then pushes that tree to a public server and at some point sends a pull request for inclusion in drm-next.

Regards,
Christian.

Am 17.07.2018 um 08:23 schrieb Zhu, Rex:

Quoted 194 lines

Patch is:

Reviewed-by Rex Zhu<[email protected] <mailto:[email protected]>>

Best Regards
Rex

------------------------------------------------------------------------
*From:* [email protected] <[email protected]> on behalf of Kees Cook <[email protected]> *Sent:* Tuesday, July 17, 2018 11:59 AM
*To:* Deucher, Alexander
*Cc:* LKML; Koenig, Christian; Zhou, David(ChunMing); David Airlie; Zhu, Rex; Huang, Ray; Kuehling, Felix; amd-gfx list; Maling list - DRI developers *Subject:* Re: [PATCH] drm/amdgpu/pm: Remove VLA usage

Quoted 4 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum sane buffer size and removes copy/paste code.

[1]

https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Quoted 2 lines

Friendly ping! Who's tree should this go through?

Thanks!

-Kees

Review

Quoted 5 lines

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++++++++++--------------

  1 file changed, 42 insertions(+), 58 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_get_pp_dpm_sclk(struct

Quoted 4 lines

index b455da487782..5eb98cde22ed 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +0 -0
@@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct 

device *dev,

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,

Quoted 9 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +2 -4
                  return snprintf(buf, PAGE_SIZE, "\n");
   }
 
-static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
-               struct device_attribute *attr,
-               const char *buf,
-               size_t count)
+/*
+ * Worst case: 32 bits individually specified, in octal at 12 

characters

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,

Quoted 5 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +5 -0
+ * per line (+1 for \n).
+ */
+#define AMDGPU_MASK_BUF_MAX    (32 * 13)
+
+static ssize_t amdgpu_read_mask(const char *buf, size_t count, 

uint32_t *mask)

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_set_pp_dpm_mclk(struct

Quoted 63 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +29 -13
   {
-       struct drm_device *ddev = dev_get_drvdata(dev);
-       struct amdgpu_device *adev = ddev->dev_private;
          int ret;
          long level;
-       uint32_t mask = 0;
          char *sub_str = NULL;
          char *tmp;
-       char buf_cpy[count];
+       char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
          const char delimiter[3] = {' ', '\n', '\0'};
+       size_t bytes;
 
-       memcpy(buf_cpy, buf, count+1);
+       *mask = 0;
+
+       bytes = min(count, sizeof(buf_cpy) - 1);
+       memcpy(buf_cpy, buf, bytes);
+       buf_cpy[bytes] = '\0';
          tmp = buf_cpy;
          while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
+               sub_str = strsep(&tmp, delimiter);
                  if (strlen(sub_str)) {
                          ret = kstrtol(sub_str, 0, &level);
-
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
+                       if (ret)
+                               return -EINVAL;
+                       *mask |= 1 << level;
                  } else
                          break;
          }
+
+       return 0;
+}
+
+static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
+               struct device_attribute *attr,
+               const char *buf,
+               size_t count)
+{
+       struct drm_device *ddev = dev_get_drvdata(dev);
+       struct amdgpu_device *adev = ddev->dev_private;
+       int ret;
+       uint32_t mask = 0;
+
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
+
          if (adev->powerplay.pp_funcs->force_clock_level)
 amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
 
-fail:
          return count;
   }
 
@@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct 

device *dev,

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_set_pp_dpm_pcie(struct

Quoted 36 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +3 -20
          struct drm_device *ddev = dev_get_drvdata(dev);
          struct amdgpu_device *adev = ddev->dev_private;
          int ret;
-       long level;
          uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
 
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
          if (adev->powerplay.pp_funcs->force_clock_level)
 amdgpu_dpm_force_clock_level(adev, PP_MCLK, mask);
 
-fail:
          return count;
   }
 
@@ -701,33 +703,15 @@ static ssize_t amdgpu_set_pp_dpm_pcie(struct 

device *dev,

Quoted 36 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +3 -21
          struct drm_device *ddev = dev_get_drvdata(dev);
          struct amdgpu_device *adev = ddev->dev_private;
          int ret;
-       long level;
          uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
-
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
 
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
          if (adev->powerplay.pp_funcs->force_clock_level)
 amdgpu_dpm_force_clock_level(adev, PP_PCIE, mask);
 
-fail:
          return count;
   }
 
Quoted 19 lines
Quoted 7 lines

--
2.17.1

--
Kees Cook
Pixel Security

Signature · 8 lines
Kees Cook
Pixel Security


_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reviewed-by Rex Zhu
#7 lore raw
In reply to a message not in the archive

Patch has been applied to our internal amd-staging-drm-next tree.

Thanks.

Best Regards

Rex

________________________________
From: Christian K?nig <[email protected]>
Sent: Tuesday, July 17, 2018 2:58:21 PM
To: Zhu, Rex; Kees Cook; Deucher, Alexander
Cc: Zhou, David(ChunMing); David Airlie; Kuehling, Felix; LKML; amd-gfx list; Huang, Ray; Maling list - DRI developers; Koenig, Christian
Subject: Re: [PATCH] drm/amdgpu/pm: Remove VLA usage

Who's tree should this go through?
To answer the question: When Rex is ok with that he pushes it to our internal amd-staging-drm-next tree.

Alex then pushes that tree to a public server and at some point sends a pull request for inclusion in drm-next.

Regards,
Christian.

Am 17.07.2018 um 08:23 schrieb Zhu, Rex:
Patch is:

Best Regards
Rex

________________________________
From: [email protected]<mailto:[email protected]> <[email protected]><mailto:[email protected]> on behalf of Kees Cook <[email protected]><mailto:[email protected]>
Sent: Tuesday, July 17, 2018 11:59 AM
To: Deucher, Alexander
Cc: LKML; Koenig, Christian; Zhou, David(ChunMing); David Airlie; Zhu, Rex; Huang, Ray; Kuehling, Felix; amd-gfx list; Maling list - DRI developers
Subject: Re: [PATCH] drm/amdgpu/pm: Remove VLA usage

Quoted 6 lines · On Wed, Jun 20, 2018 at 11:26 AM, Kees Cook…

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum sane buffer size and removes copy/paste code.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Friendly ping! Who's tree should this go through?

Thanks!

-Kees

Quoted 162 lines
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++++++++++--------------
 1 file changed, 42 insertions(+), 58 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +42 -59
index b455da487782..5eb98cde22ed 100644
@@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
                return snprintf(buf, PAGE_SIZE, "\n");
 }
 
-static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
-               struct device_attribute *attr,
-               const char *buf,
-               size_t count)
+/*
+ * Worst case: 32 bits individually specified, in octal at 12 characters
+ * per line (+1 for \n).
+ */
+#define AMDGPU_MASK_BUF_MAX    (32 * 13)
+
+static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t *mask)
 {
-       struct drm_device *ddev = dev_get_drvdata(dev);
-       struct amdgpu_device *adev = ddev->dev_private;
        int ret;
        long level;
-       uint32_t mask = 0;
        char *sub_str = NULL;
        char *tmp;
-       char buf_cpy[count];
+       char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
        const char delimiter[3] = {' ', '\n', '\0'};
+       size_t bytes;
 
-       memcpy(buf_cpy, buf, count+1);
+       *mask = 0;
+
+       bytes = min(count, sizeof(buf_cpy) - 1);
+       memcpy(buf_cpy, buf, bytes);
+       buf_cpy[bytes] = '\0';
        tmp = buf_cpy;
        while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
+               sub_str = strsep(&tmp, delimiter);
                if (strlen(sub_str)) {
                        ret = kstrtol(sub_str, 0, &level);
-
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
+                       if (ret)
+                               return -EINVAL;
+                       *mask |= 1 << level;
                } else
                        break;
        }
+
+       return 0;
+}
+
+static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
+               struct device_attribute *attr,
+               const char *buf,
+               size_t count)
+{
+       struct drm_device *ddev = dev_get_drvdata(dev);
+       struct amdgpu_device *adev = ddev->dev_private;
+       int ret;
+       uint32_t mask = 0;
+
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
+
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
 
-fail:
        return count;
 }
 
@@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device *dev,
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;
        int ret;
-       long level;
        uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
 
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_MCLK, mask);
 
-fail:
        return count;
 }
 
@@ -701,33 +703,15 @@ static ssize_t amdgpu_set_pp_dpm_pcie(struct device *dev,
        struct drm_device *ddev = dev_get_drvdata(dev);
        struct amdgpu_device *adev = ddev->dev_private;
        int ret;
-       long level;
        uint32_t mask = 0;
-       char *sub_str = NULL;
-       char *tmp;
-       char buf_cpy[count];
-       const char delimiter[3] = {' ', '\n', '\0'};
-
-       memcpy(buf_cpy, buf, count+1);
-       tmp = buf_cpy;
 
-       while (tmp[0]) {
-               sub_str =  strsep(&tmp, delimiter);
-               if (strlen(sub_str)) {
-                       ret = kstrtol(sub_str, 0, &level);
+       ret = amdgpu_read_mask(buf, count, &mask);
+       if (ret)
+               return ret;
 
-                       if (ret) {
-                               count = -EINVAL;
-                               goto fail;
-                       }
-                       mask |= 1 << level;
-               } else
-                       break;
-       }
        if (adev->powerplay.pp_funcs->force_clock_level)
                amdgpu_dpm_force_clock_level(adev, PP_PCIE, mask);
 
-fail:
        return count;
 }
 
--

2.17.1

Quoted 3 lines

--
Kees Cook
Pixel Security

--
Kees Cook
Pixel Security

_______________________________________________
amd-gfx mailing list [email protected]<mailto:[email protected]>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx