Re: [PATCH 2/2] sysfs: only access bin file vm_ops with the activelock

[Hugh Dickins]

On Mon, 20 Sep 2010, Eric W. Biederman wrote:
> 
> bb->vm_ops is a cached copy of the vm_ops of the underlying
> sysfs bin file, which means that after sysfs_bin_remove_file
> completes it is only longer valid to deference bb->vm_ops.
> 
> So move all of the tests of bb->vm_ops inside of where
> we hold the sysfs active lock.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx>

Good point.  There's a couple of cases (the ones below) where you've
changed the return value from success to failure when the entry point
didn't exist for an object that is no longer active; but on reflection,
I accept that's just not worth worrying about.

Hugh

> @@ -219,13 +222,12 @@ static int bin_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	if (!bb->vm_ops)
>  		return VM_FAULT_SIGBUS;
>  
> -	if (!bb->vm_ops->page_mkwrite)
> -		return 0;
> -
>  	if (!sysfs_get_active(attr_sd))
>  		return VM_FAULT_SIGBUS;
>  
> -	ret = bb->vm_ops->page_mkwrite(vma, vmf);
> +	ret = 0;
> +	if (bb->vm_ops->page_mkwrite)
> +		ret = bb->vm_ops->page_mkwrite(vma, vmf);
>  
>  	sysfs_put_active(attr_sd);
>  	return ret;

> @@ -259,13 +263,15 @@ static int bin_set_policy(struct vm_area_struct *vma, struct mempolicy *new)
>  	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
>  	int ret;
>  
> -	if (!bb->vm_ops || !bb->vm_ops->set_policy)
> +	if (!bb->vm_ops)
>  		return 0;
>  
>  	if (!sysfs_get_active(attr_sd))
>  		return -EINVAL;
>  
> -	ret = bb->vm_ops->set_policy(vma, new);
> +	ret = 0;
> +	if (bb->vm_ops->set_policy)
> +		ret = bb->vm_ops->set_policy(vma, new);
>  
>  	sysfs_put_active(attr_sd);
>  	return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/