Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 6 Nov 2018 12:17:55 -0800
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Jerry Lin <wahahab11@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>, ghartman@google.com,
        astrachan@google.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] driver-staging: vsoc.c: Add sysfs support for
 examining the permissions of regions.
Message-ID: <20181106201755.GA204219@google.com>
References: <20181106052116.GA25953@ubuntu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181106052116.GA25953@ubuntu>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 06, 2018 at 01:21:16PM +0800, Jerry Lin wrote:
> Add a attribute called permissions under vsoc device node for examining
> current granted permissions in vsoc_device.
> 
> This file will display permissions in following format:
>   begin_offset  end_offset  owner_offset  owned_value
>             %x          %x            %x           %x
> 
> Signed-off-by: Jerry Lin <wahahab11@gmail.com>

Please always post patches to the list with reviewers on CC. Also CC the
following addresses in addition:
astrachan@google.com
ghartman@google.com (infact you deleted a TODO he added so)

And just one more minor nit below, otherwise LGTM.

thanks,

- Joel

> ---
>  drivers/staging/android/vsoc.c | 49 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/android/vsoc.c b/drivers/staging/android/vsoc.c
> index 22571ab..481554a 100644
> --- a/drivers/staging/android/vsoc.c
> +++ b/drivers/staging/android/vsoc.c
> @@ -128,9 +128,10 @@ struct vsoc_device {
>  
>  static struct vsoc_device vsoc_dev;
>  
> -/*
> - * TODO(ghartman): Add a /sys filesystem entry that summarizes the permissions.
> - */
> +static ssize_t permissions_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf);
> +static DEVICE_ATTR_RO(permissions);
>  
>  struct fd_scoped_permission_node {
>  	struct fd_scoped_permission permission;
> @@ -718,6 +719,38 @@ static ssize_t vsoc_write(struct file *filp, const char __user *buffer,
>  	return len;
>  }
>  
> +static ssize_t permissions_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buffer)
> +{
> +	struct fd_scoped_permission_node *node;
> +	char *row;
> +	int ret;
> +	ssize_t written = 0;
> +
> +	row = kzalloc(sizeof(char) * 128, GFP_KERNEL);
> +	if (!row)
> +		return 0;
> +	mutex_lock(&vsoc_dev.mtx);
> +	list_for_each_entry(node, &vsoc_dev.permissions, list) {
> +		ret = snprintf(row, 128, "%x\t%x\t%x\t%x\n",
> +			       node->permission.begin_offset,
> +			       node->permission.end_offset,
> +			       node->permission.owner_offset,
> +			       node->permission.owned_value);
> +		if (ret < 0)
> +			goto done;
> +		row[ret] = '\0';

Do you really need null termination after snprintf?

> +		memcpy(buffer + written, row, ret);
> +		written += ret;
> +	}
> +
> +done:
> +	mutex_unlock(&vsoc_dev.mtx);
> +	kfree(row);
> +	return written;
> +}
> +
>  static irqreturn_t vsoc_interrupt(int irq, void *region_data_v)
>  {
>  	struct vsoc_region_data *region_data =
> @@ -942,6 +975,15 @@ static int vsoc_probe_device(struct pci_dev *pdev,
>  		}
>  		vsoc_dev.regions_data[i].device_created = true;
>  	}
> +	/*
> +	 * Create permission attribute on device node.
> +	 */
> +	result = device_create_file(&pdev->dev, &dev_attr_permissions);
> +	if (result) {
> +		dev_err(&vsoc_dev.dev->dev, "device_create_file failed\n");
> +		vsoc_remove_device(pdev);
> +		return -EFAULT;
> +	}
>  	return 0;
>  }
>  
> @@ -967,6 +1009,7 @@ static void vsoc_remove_device(struct pci_dev *pdev)
>  	if (!pdev || !vsoc_dev.dev)
>  		return;
>  	dev_info(&pdev->dev, "remove_device\n");
> +	device_remove_file(&pdev->dev, &dev_attr_permissions);
>  	if (vsoc_dev.regions_data) {
>  		for (i = 0; i < vsoc_dev.layout->region_count; ++i) {
>  			if (vsoc_dev.regions_data[i].device_created) {
> -- 
> 2.7.4
>