[PATCH 0/4] AMDKFD (AMD GPU compute) support for device cgroup.

[PATCH 0/4] AMDKFD (AMD GPU compute) support for device cgroup.

[Kasiviswanathan, Harish]

amdkfd (part of amdgpu) driver supports the AMD GPU compute stack.
amdkfd exposes only a single device /dev/kfd even if multiple AMD GPU
(compute) devices exist in a system. However, amdgpu drvier exposes a
separate render device file /dev/dri/renderDN for each device. To participate
in device cgroup amdkfd driver will rely on these redner device files.

Harish Kasiviswanathan (4):
  drm/amdkfd: Store kfd_dev in iolink and cache properties
  drm/amd: Pass drm_device to kfd
  device_cgroup: Export __devcgroup_check_permission
  drm/amdkfd: Check against device cgroup

 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h   |  1 +
 drivers/gpu/drm/amd/amdkfd/kfd_device.c      |  2 ++
 drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c |  9 ++++++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h        | 20 ++++++++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c    | 22 ++++++++++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.h    |  3 +++
 security/device_cgroup.c                     |  1 +
 8 files changed, 57 insertions(+), 3 deletions(-)

-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/4] drm/amdkfd: Store kfd_dev in iolink and cache properties

[Kasiviswanathan, Harish]

This is required to check against cgroup permissions.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 10 ++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.h |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 2c06d6c16eab..64d667ae0d36 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1104,6 +1104,9 @@ static struct kfd_topology_device *kfd_assign_gpu(struct kfd_dev *gpu)
 {
 	struct kfd_topology_device *dev;
 	struct kfd_topology_device *out_dev = NULL;
+	struct kfd_mem_properties *mem;
+	struct kfd_cache_properties *cache;
+	struct kfd_iolink_properties *iolink;
 
 	down_write(&topology_lock);
 	list_for_each_entry(dev, &topology_device_list, list) {
@@ -1117,6 +1120,13 @@ static struct kfd_topology_device *kfd_assign_gpu(struct kfd_dev *gpu)
 		if (!dev->gpu && (dev->node_props.simd_count > 0)) {
 			dev->gpu = gpu;
 			out_dev = dev;
+
+			list_for_each_entry(mem, &dev->mem_props, list)
+				mem->gpu = dev->gpu;
+			list_for_each_entry(cache, &dev->cache_props, list)
+				cache->gpu = dev->gpu;
+			list_for_each_entry(iolink, &dev->io_link_props, list)
+				iolink->gpu = dev->gpu;
 			break;
 		}
 	}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
index 949e885dfb53..5fd3df80bb0e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
@@ -101,6 +101,7 @@ struct kfd_mem_properties {
 	uint32_t		flags;
 	uint32_t		width;
 	uint32_t		mem_clk_max;
+	struct kfd_dev		*gpu;
 	struct kobject		*kobj;
 	struct attribute	attr;
 };
@@ -122,6 +123,7 @@ struct kfd_cache_properties {
 	uint32_t		cache_latency;
 	uint32_t		cache_type;
 	uint8_t			sibling_map[CRAT_SIBLINGMAP_SIZE];
+	struct kfd_dev		*gpu;
 	struct kobject		*kobj;
 	struct attribute	attr;
 };
@@ -140,6 +142,7 @@ struct kfd_iolink_properties {
 	uint32_t		max_bandwidth;
 	uint32_t		rec_transfer_size;
 	uint32_t		flags;
+	struct kfd_dev		*gpu;
 	struct kobject		*kobj;
 	struct attribute	attr;
 };
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/4] drm/amd: Pass drm_device to kfd

[Kasiviswanathan, Harish]

kfd needs drm_device to call into drm_cgroup functions

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 1 +
 drivers/gpu/drm/amd/amdkfd/kfd_device.c    | 2 ++
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h      | 3 +++
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 8949b1ac2f18..9433312f789c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -190,7 +190,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
 					adev->doorbell_index.last_non_cp;
 		}
 
-		kgd2kfd_device_init(adev->kfd.dev, &gpu_resources);
+		kgd2kfd_device_init(adev->kfd.dev, adev->ddev, &gpu_resources);
 	}
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index f57f29763769..bad378596f46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -234,6 +234,7 @@ void kgd2kfd_exit(void);
 struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd, struct pci_dev *pdev,
 			      const struct kfd2kgd_calls *f2g);
 bool kgd2kfd_device_init(struct kfd_dev *kfd,
+			 struct drm_device *ddev,
 			 const struct kgd2kfd_shared_resources *gpu_resources);
 void kgd2kfd_device_exit(struct kfd_dev *kfd);
 void kgd2kfd_suspend(struct kfd_dev *kfd);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index a53dda9071b1..7f5d62719e40 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -526,10 +526,12 @@ static void kfd_cwsr_init(struct kfd_dev *kfd)
 }
 
 bool kgd2kfd_device_init(struct kfd_dev *kfd,
+			 struct drm_device *ddev,
 			 const struct kgd2kfd_shared_resources *gpu_resources)
 {
 	unsigned int size;
 
+	kfd->ddev = ddev;
 	kfd->mec_fw_version = amdgpu_amdkfd_get_fw_version(kfd->kgd,
 			KGD_ENGINE_MEC1);
 	kfd->sdma_fw_version = amdgpu_amdkfd_get_fw_version(kfd->kgd,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index c8925b7b6c46..7caf4fa3d366 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -46,6 +46,8 @@
 /* GPU ID hash width in bits */
 #define KFD_GPU_ID_HASH_WIDTH 16
 
+struct drm_device;
+
 /* Use upper bits of mmap offset to store KFD driver specific information.
  * BITS[63:62] - Encode MMAP type
  * BITS[61:46] - Encode gpu_id. To identify to which GPU the offset belongs to
@@ -211,6 +213,7 @@ struct kfd_dev {
 
 	const struct kfd_device_info *device_info;
 	struct pci_dev *pdev;
+	struct drm_device *ddev;
 
 	unsigned int id;		/* topology stub index */
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] device_cgroup: Export __devcgroup_check_permission

[Kasiviswanathan, Harish]

For AMD compute (amdkfd) driver.

All AMD compute devices are exported via single device node /dev/kfd. As
a result devices cannot be controlled individually using device cgroup.

AMD compute devices will rely on its graphics counterpart that exposes
/dev/dri/renderN node for each device. For each task (based on its
cgroup), KFD driver will check if /dev/dri/renderN node is accessible
before exposing it.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 security/device_cgroup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index cd97929fac66..e3a9ad5db5a0 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -824,3 +824,4 @@ int __devcgroup_check_permission(short type, u32 major, u32 minor,
 
 	return 0;
 }
+EXPORT_SYMBOL(__devcgroup_check_permission);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/4] device_cgroup: Export __devcgroup_check_permission

[Kasiviswanathan, Harish]

Hello Everyone,

Could you please review this patch?

Best Regards,

Harish



On 2019-05-01 10:59 a.m., Kasiviswanathan, Harish wrote:
> For AMD compute (amdkfd) driver.
>
> All AMD compute devices are exported via single device node /dev/kfd. As
> a result devices cannot be controlled individually using device cgroup.
>
> AMD compute devices will rely on its graphics counterpart that exposes
> /dev/dri/renderN node for each device. For each task (based on its
> cgroup), KFD driver will check if /dev/dri/renderN node is accessible
> before exposing it.
>
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  security/device_cgroup.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index cd97929fac66..e3a9ad5db5a0 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -824,3 +824,4 @@ int __devcgroup_check_permission(short type, u32 major, u32 minor,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(__devcgroup_check_permission);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/4] drm/amdkfd: Check against device cgroup

[Kasiviswanathan, Harish]

Participate in device cgroup. All kfd devices are exposed via /dev/kfd.
So use /dev/dri/renderN node.

Before exposing the device to a task check if it has permission to
access it. If the task (based on its cgroup) can access /dev/dri/renderN
then expose the device via kfd node.

If the task cannot access /dev/dri/renderN then process device data
(pdd) is not created. This will ensure that task cannot use the device.

In sysfs topology, all device nodes are visible irrespective of the task
cgroup. The sysfs node directories are created at driver load time and
cannot be changed dynamically. However, access to information inside
nodes is controlled based on the task's cgroup permissions.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c |  9 +++++++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h        | 17 +++++++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c    | 12 ++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
index dc7339825b5c..3804edfb4ff7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
@@ -369,8 +369,13 @@ int kfd_init_apertures(struct kfd_process *process)
 
 	/*Iterating over all devices*/
 	while (kfd_topology_enum_kfd_devices(id, &dev) == 0) {
-		if (!dev) {
-			id++; /* Skip non GPU devices */
+		if (!dev || kfd_devcgroup_check_permission(dev)) {
+			/* Skip non GPU devices and devices to which the
+			 * current process have no access to. Access can be
+			 * limited by placing the process in a specific
+			 * cgroup hierarchy
+			 */
+			id++;
 			continue;
 		}
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7caf4fa3d366..8b40922da0fd 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -35,6 +35,8 @@
 #include <linux/kfifo.h>
 #include <linux/seq_file.h>
 #include <linux/kref.h>
+#include <linux/device_cgroup.h>
+#include <drm/drmP.h>
 #include <kgd_kfd_interface.h>
 
 #include "amd_shared.h"
@@ -989,6 +991,21 @@ bool kfd_is_locked(void);
 void kfd_inc_compute_active(struct kfd_dev *dev);
 void kfd_dec_compute_active(struct kfd_dev *dev);
 
+/* Cgroup Support */
+/* Check with device cgroup if @kfd device is accessible */
+static inline int kfd_devcgroup_check_permission(struct kfd_dev *kfd)
+{
+#if defined(CONFIG_CGROUP_DEVICE)
+	struct drm_device *ddev = kfd->ddev;
+
+	return devcgroup_check_permission(DEVCG_DEV_CHAR, ddev->driver->major,
+					  ddev->render->index,
+					  DEVCG_ACC_WRITE | DEVCG_ACC_READ);
+#else
+	return 0;
+#endif
+}
+
 /* Debugfs */
 #if defined(CONFIG_DEBUG_FS)
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 64d667ae0d36..a3a14a76ece1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -269,6 +269,8 @@ static ssize_t iolink_show(struct kobject *kobj, struct attribute *attr,
 	buffer[0] = 0;
 
 	iolink = container_of(attr, struct kfd_iolink_properties, attr);
+	if (iolink->gpu && kfd_devcgroup_check_permission(iolink->gpu))
+		return -EPERM;
 	sysfs_show_32bit_prop(buffer, "type", iolink->iolink_type);
 	sysfs_show_32bit_prop(buffer, "version_major", iolink->ver_maj);
 	sysfs_show_32bit_prop(buffer, "version_minor", iolink->ver_min);
@@ -305,6 +307,8 @@ static ssize_t mem_show(struct kobject *kobj, struct attribute *attr,
 	buffer[0] = 0;
 
 	mem = container_of(attr, struct kfd_mem_properties, attr);
+	if (mem->gpu && kfd_devcgroup_check_permission(mem->gpu))
+		return -EPERM;
 	sysfs_show_32bit_prop(buffer, "heap_type", mem->heap_type);
 	sysfs_show_64bit_prop(buffer, "size_in_bytes", mem->size_in_bytes);
 	sysfs_show_32bit_prop(buffer, "flags", mem->flags);
@@ -334,6 +338,8 @@ static ssize_t kfd_cache_show(struct kobject *kobj, struct attribute *attr,
 	buffer[0] = 0;
 
 	cache = container_of(attr, struct kfd_cache_properties, attr);
+	if (cache->gpu && kfd_devcgroup_check_permission(cache->gpu))
+		return -EPERM;
 	sysfs_show_32bit_prop(buffer, "processor_id_low",
 			cache->processor_id_low);
 	sysfs_show_32bit_prop(buffer, "level", cache->cache_level);
@@ -416,12 +422,16 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 	if (strcmp(attr->name, "gpu_id") == 0) {
 		dev = container_of(attr, struct kfd_topology_device,
 				attr_gpuid);
+		if (dev->gpu && kfd_devcgroup_check_permission(dev->gpu))
+			return -EPERM;
 		return sysfs_show_32bit_val(buffer, dev->gpu_id);
 	}
 
 	if (strcmp(attr->name, "name") == 0) {
 		dev = container_of(attr, struct kfd_topology_device,
 				attr_name);
+		if (dev->gpu && kfd_devcgroup_check_permission(dev->gpu))
+			return -EPERM;
 		for (i = 0; i < KFD_TOPOLOGY_PUBLIC_NAME_SIZE; i++) {
 			public_name[i] =
 					(char)dev->node_props.marketing_name[i];
@@ -434,6 +444,8 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 
 	dev = container_of(attr, struct kfd_topology_device,
 			attr_props);
+	if (dev->gpu && kfd_devcgroup_check_permission(dev->gpu))
+		return -EPERM;
 	sysfs_show_32bit_prop(buffer, "cpu_cores_count",
 			dev->node_props.cpu_cores_count);
 	sysfs_show_32bit_prop(buffer, "simd_count",
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdkfd: Check against device cgroup

[Roman Gushchin]

On Wed, May 01, 2019 at 02:59:29PM +0000, Kasiviswanathan, Harish wrote:
> Participate in device cgroup. All kfd devices are exposed via /dev/kfd.
> So use /dev/dri/renderN node.
> 
> Before exposing the device to a task check if it has permission to
> access it. If the task (based on its cgroup) can access /dev/dri/renderN
> then expose the device via kfd node.
> 
> If the task cannot access /dev/dri/renderN then process device data
> (pdd) is not created. This will ensure that task cannot use the device.
> 
> In sysfs topology, all device nodes are visible irrespective of the task
> cgroup. The sysfs node directories are created at driver load time and
> cannot be changed dynamically. However, access to information inside
> nodes is controlled based on the task's cgroup permissions.
> 
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Hello, Harish!

Cgroup/device controller part looks good to me.
Please, feel free to use my acks for patches 3 and 4:
Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c |  9 +++++++--
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h        | 17 +++++++++++++++++
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c    | 12 ++++++++++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> index dc7339825b5c..3804edfb4ff7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
> @@ -369,8 +369,13 @@ int kfd_init_apertures(struct kfd_process *process)
>  
>  	/*Iterating over all devices*/
>  	while (kfd_topology_enum_kfd_devices(id, &dev) == 0) {
> -		if (!dev) {
> -			id++; /* Skip non GPU devices */
> +		if (!dev || kfd_devcgroup_check_permission(dev)) {
> +			/* Skip non GPU devices and devices to which the
> +			 * current process have no access to. Access can be
> +			 * limited by placing the process in a specific
> +			 * cgroup hierarchy
                                           ^
				Probably, a missing dot here.
> +			 */
> +			id++;
>  			continue;
>  		}
>  
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdkfd: Check against device cgroup

[Roman Gushchin]

On Tue, May 14, 2019 at 01:58:40AM +0000, Roman Gushchin wrote:
> On Wed, May 01, 2019 at 02:59:29PM +0000, Kasiviswanathan, Harish wrote:
> > Participate in device cgroup. All kfd devices are exposed via /dev/kfd.
> > So use /dev/dri/renderN node.
> > 
> > Before exposing the device to a task check if it has permission to
> > access it. If the task (based on its cgroup) can access /dev/dri/renderN
> > then expose the device via kfd node.
> > 
> > If the task cannot access /dev/dri/renderN then process device data
> > (pdd) is not created. This will ensure that task cannot use the device.
> > 
> > In sysfs topology, all device nodes are visible irrespective of the task
> > cgroup. The sysfs node directories are created at driver load time and
> > cannot be changed dynamically. However, access to information inside
> > nodes is controlled based on the task's cgroup permissions.
> > 
> > Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
> > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> 
> Hello, Harish!
> 
> Cgroup/device controller part looks good to me.
> Please, feel free to use my acks for patches 3 and 4:
> Acked-by: Roman Gushchin <guro@fb.com>

Hello!

After the second look at the patchset I came to an understanding that
exporting cgroup_v1-only __devcgroup_check_permission() isn't the best idea.

Instead it would be better to export devcgroup_check_permission(), which
provides an universal interface for both cgroup v1 and v2 device controllers.
It  require some refactorings, but should be not hard.

Does it makes sense to you? Can you, please, rework this part?

Thanks!
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdkfd: Check against device cgroup

[Kasiviswanathan, Harish]

Hi Roman,

Thanks for your feedback. I will rework and send new patch soon.

Best Regards,
Harish
 




From: Roman Gushchin <guro@fb.com>
Sent: Tuesday, May 14, 2019 1:37 PM
To: Kasiviswanathan, Harish
Cc: cgroups@vger.kernel.org; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/4] drm/amdkfd: Check against device cgroup
  

[CAUTION: External Email]

On Tue, May 14, 2019 at 01:58:40AM +0000, Roman Gushchin wrote:
> On Wed, May 01, 2019 at 02:59:29PM +0000, Kasiviswanathan, Harish wrote:
> > Participate in device cgroup. All kfd devices are exposed via /dev/kfd.
> > So use /dev/dri/renderN node.
> >
> > Before exposing the device to a task check if it has permission to
> > access it. If the task (based on its cgroup) can access /dev/dri/renderN
> > then expose the device via kfd node.
> >
> > If the task cannot access /dev/dri/renderN then process device data
> > (pdd) is not created. This will ensure that task cannot use the device.
> >
> > In sysfs topology, all device nodes are visible irrespective of the task
> > cgroup. The sysfs node directories are created at driver load time and
> > cannot be changed dynamically. However, access to information inside
> > nodes is controlled based on the task's cgroup permissions.
> >
> > Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
> > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>
> Hello, Harish!
>
> Cgroup/device controller part looks good to me.
> Please, feel free to use my acks for patches 3 and 4:
> Acked-by: Roman Gushchin <guro@fb.com>

Hello!

After the second look at the patchset I came to an understanding that
exporting cgroup_v1-only __devcgroup_check_permission() isn't the best idea.

Instead it would be better to export devcgroup_check_permission(), which
provides an universal interface for both cgroup v1 and v2 device controllers.
It  require some refactorings, but should be not hard.

Does it makes sense to you? Can you, please, rework this part?

Thanks!
    
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx