Re: [PATCH] virt: acrn: Fix document of acrn_msi_inject()

[Liu, Shuo A]

On 3/10/2021 23:50, Greg Kroah-Hartman wrote:
> On Wed, Mar 10, 2021 at 11:37:51PM +0800, shuo.a@intel.com wrote:
>> From: Shuo Liu 
>>
>> This fixes below sparse warning.
>>
>> ../drivers/virt/acrn/vm.c:105: warning: expecting prototype for
>> acrn_inject_msi(). Prototype was for acrn_msi_inject() instead
> 
> That is not a warning from sparse :(
> 

Oh. You are right. I was fixing another sparse warning and did
make O=out -j24 W=1 C=1
to reproduce that warning.

It misled me that this warning coming out with sparse too.

Let me re-send the patch.

Thanks
shuo

Re: [PATCH] virt: acrn: Use vfs_poll() instead of f_op->poll()

[Liu, Shuo A]

On 2/20/2021 22:53, Yejune Deng wrote:
> Use vfs_poll() is a more advanced function in acrn_irqfd_assign().
> as the same time, modify the definition of events.
> 
> Signed-off-by: Yejune Deng 

Thanks for the update.
Reviewed-by: Shuo Liu 

Hi Greg,
Need i do more work on this patch?
Or you will review and apply on your tree directly?

Thanks
shuo

> ---
>  drivers/virt/acrn/irqfd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virt/acrn/irqfd.c b/drivers/virt/acrn/irqfd.c
> index a8766d528e29..98d6e9b18f9e 100644
> --- a/drivers/virt/acrn/irqfd.c
> +++ b/drivers/virt/acrn/irqfd.c
> @@ -112,7 +112,7 @@ static int acrn_irqfd_assign(struct acrn_vm *vm, struct 
> acrn_irqfd *args)
>  {
>   struct eventfd_ctx *eventfd = NULL;
>   struct hsm_irqfd *irqfd, *tmp;
> - unsigned int events;
> + __poll_t events;
>   struct fd f;
>   int ret = 0;
>  
> @@ -158,7 +158,7 @@ static int acrn_irqfd_assign(struct acrn_vm *vm, struct 
> acrn_irqfd *args)
>   mutex_unlock(>irqfds_lock);
>  
>   /* Check the pending event in this stage */
> - events = f.file->f_op->poll(f.file, >pt);
> + events = vfs_poll(f.file, >pt);
>  
>   if (events & POLLIN)
>   acrn_irqfd_inject(irqfd);
> 

Re: linux-next: Tree for Feb 10 (acrn)

[Liu, Shuo A]

On 2/11/2021 01:52, Randy Dunlap wrote:
> On 2/10/21 3:42 AM, Stephen Rothwell wrote:
>> Hi all,
>>
>> Changes since 20210209:
>>
> 
> ../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
> ../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function 
> ‘remove_cpu’; did you mean ‘register_cpu’? 
> [-Werror=implicit-function-declaration]
>remove_cpu(cpu);
> 
> ../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function 
> ‘add_cpu’; did you mean ‘task_cpu’? [-Werror=implicit-function-declaration]
>   add_cpu(cpu);
> 
> 
> Full randconfig file is attached.
> 

Thanks.
The vCPU removing depends on CONFIG_HOTPLUG_CPU. Below change could fix.

---

diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
index 1f6b7c54a1a4..e340788aacdf 100644
--- a/drivers/virt/acrn/hsm.c
+++ b/drivers/virt/acrn/hsm.c
@@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode,
struct file *filp)
return 0;
 }

+#ifdef CONFIG_HOTPLUG_CPU
 static ssize_t remove_cpu_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
return ret;
 }
 static DEVICE_ATTR_WO(remove_cpu);
+#endif

 static struct attribute *acrn_attrs[] = {
+#ifdef CONFIG_HOTPLUG_CPU
_attr_remove_cpu.attr,
+#endif
NULL
 };

Re: [PATCH v4 00/17] HSM driver for ACRN hypervisor

[Liu, Shuo A]

Ping...

On 9/22/2020 19:42, shuo.a@intel.com wrote:
> From: Shuo Liu 
> 
> ACRN is a Type 1 reference hypervisor stack, running directly on the 
> bare-metal
> hardware, and is suitable for a variety of IoT and embedded device solutions.
> 
> ACRN implements a hybrid VMM architecture, using a privileged Service VM. The
> Service VM manages the system resources (CPU, memory, etc.) and I/O devices of
> User VMs. Multiple User VMs are supported, with each of them running Linux,
> Android OS or Windows. Both Service VM and User VMs are guest VM.
> 
> Below figure shows the architecture.
> 
> Service VMUser VM
>   ++  |  +--+
>   |+--+|  |  |  |
>   ||ACRN userspace||  |  |  |
>   |+--+|  |  |  |
>   |-ioctl--|  |  |  |   ...
>   |kernel space   +--+ |  |  |  |
>   |   |   HSM| |  |  | Drivers  |
>   |   +--+ |  |  |  |
>   +|---+  |  +--+
>   +-hypercall+
>   |   ACRN Hypervisor|
>   +--+
>   |  Hardware|
>   +--+
> 
> There is only one Service VM which could run Linux as OS.
> 
> In a typical case, the Service VM will be auto started when ACRN Hypervisor is
> booted. Then the ACRN userspace (an application running in Service VM) could 
> be
> used to start/stop User VMs by communicating with ACRN Hypervisor Service
> Module (HSM).
> 
> ACRN Hypervisor Service Module (HSM) is a middle layer that allows the ACRN
> userspace and Service VM OS kernel to communicate with ACRN Hypervisor
> and manage different User VMs. This middle layer provides the following
> functionalities,
>   - Issues hypercalls to the hypervisor to manage User VMs:
>   * VM/vCPU management
>   * Memory management
>   * Device passthrough
>   * Interrupts injection
>   - I/O requests handling from User VMs.
>   - Exports ioctl through HSM char device.
>   - Exports function calls for other kernel modules
> 
> ACRN is focused on embedded system. So it doesn't support some features.
> E.g.,
>   - ACRN doesn't support VM migration.
>   - ACRN doesn't support vCPU migration.
> 
> This patch set adds the HSM to the Linux kernel.
> 
> The basic ARCN support was merged to upstream already.
> https://lore.kernel.org/lkml/1559108037-18813-3-git-send-email-yakui.z...@intel.com/
> 
> ChangeLog:
> v4:
>   - Used acrn_dev.this_device directly for dev_*() (Reinette)
>   - Removed the odd usage of {get|put}_device() on _dev->this_device 
> (Greg)
>   - Removed unused log code. (Greg)
>   - Corrected the return error values. (Greg)
>   - Mentioned that HSM relies hypervisor for sanity check in acrn_dev_ioctl() 
> comments (Greg)
> 
> v3:
>   - Used {get|put}_device() helpers on _dev->this_device
>   - Moved unused code from front patches to later ones.
>   - Removed self-defined pr_fmt() and dev_fmt()
>   - Provided comments for acrn_vm_list_lock.
> 
> v2:
>   - Removed API version related code. (Dave)
>   - Replaced pr_*() by dev_*(). (Greg)
>   - Used -ENOTTY as the error code of unsupported ioctl. (Greg)
> 
> Shuo Liu (16):
>   docs: acrn: Introduce ACRN
>   x86/acrn: Introduce acrn_{setup, remove}_intr_handler()
>   x86/acrn: Introduce hypercall interfaces
>   virt: acrn: Introduce ACRN HSM basic driver
>   virt: acrn: Introduce VM management interfaces
>   virt: acrn: Introduce an ioctl to set vCPU registers state
>   virt: acrn: Introduce EPT mapping management
>   virt: acrn: Introduce I/O request management
>   virt: acrn: Introduce PCI configuration space PIO accesses combiner
>   virt: acrn: Introduce interfaces for PCI device passthrough
>   virt: acrn: Introduce interrupt injection interfaces
>   virt: acrn: Introduce interfaces to query C-states and P-states
> allowed by hypervisor
>   virt: acrn: Introduce I/O ranges operation interfaces
>   virt: acrn: Introduce ioeventfd
>   virt: acrn: Introduce irqfd
>   virt: acrn: Introduce an interface for Service VM to control vCPU
> 
> Yin Fengwei (1):
>   x86/acrn: Introduce an API to check if a VM is privileged
> 
>  .../userspace-api/ioctl/ioctl-number.rst  |   1 +
>  Documentation/virt/acrn/index.rst |  11 +
>  Documentation/virt/acrn/introduction.rst  |  40 ++
>  Documentation/virt/acrn/io-request.rst|  97 +++
>  Documentation/virt/index.rst  |   1 +
>  MAINTAINERS   |   9 +
>  arch/x86/include/asm/acrn.h  

Re: [PATCH v3 06/17] virt: acrn: Introduce VM management interfaces

[Liu, Shuo A]

Hi Greg,

On 9/11/2020 00:28, Greg Kroah-Hartman wrote:
> On Thu, Sep 10, 2020 at 02:19:00PM +0800, Shuo A Liu wrote:
>> On Wed  9.Sep'20 at 11:45:16 +0200, Greg Kroah-Hartman wrote:
>>> On Wed, Sep 09, 2020 at 05:08:25PM +0800, shuo.a@intel.com wrote:
 From: Shuo Liu 

 The VM management interfaces expose several VM operations to ACRN
 userspace via ioctls. For example, creating VM, starting VM, destroying
 VM and so on.

 The ACRN Hypervisor needs to exchange data with the ACRN userspace
 during the VM operations. HSM provides VM operation ioctls to the ACRN
 userspace and communicates with the ACRN Hypervisor for VM operations
 via hypercalls.

 HSM maintains a list of User VM. Each User VM will be bound to an
 existing file descriptor of /dev/acrn_hsm. The User VM will be
 destroyed when the file descriptor is closed.

 Signed-off-by: Shuo Liu 
 Reviewed-by: Zhi Wang 
 Reviewed-by: Reinette Chatre 
 Cc: Zhi Wang 
 Cc: Zhenyu Wang 
 Cc: Yu Wang 
 Cc: Reinette Chatre 
 Cc: Greg Kroah-Hartman 
 ---
  .../userspace-api/ioctl/ioctl-number.rst  |  1 +
  MAINTAINERS   |  1 +
  drivers/virt/acrn/Makefile|  2 +-
  drivers/virt/acrn/acrn_drv.h  | 22 +-
  drivers/virt/acrn/hsm.c   | 66 
  drivers/virt/acrn/hypercall.h | 78 +++
  drivers/virt/acrn/vm.c| 69 
  include/uapi/linux/acrn.h | 56 +
  8 files changed, 293 insertions(+), 2 deletions(-)
  create mode 100644 drivers/virt/acrn/hypercall.h
  create mode 100644 drivers/virt/acrn/vm.c
  create mode 100644 include/uapi/linux/acrn.h


[...]

 +  ret = hcall_create_vm(virt_to_phys(vm_param));
 +  if (ret < 0 || vm_param->vmid == ACRN_INVALID_VMID) {
 +  dev_err(vm->dev, "Failed to create VM! Error: %d\n", ret);
 +  return NULL;
 +  }
 +
 +  vm->vmid = vm_param->vmid;
 +  vm->vcpu_num = vm_param->vcpu_num;
 +
 +  write_lock_bh(_vm_list_lock);
 +  list_add(>list, _vm_list);
>>>
>>> Wait, why do you have a global list of devices?  Shouldn't that device
>>> be tied to the vm structure?  Who will be iterating this list that does
>>> not have the file handle to start with?
>>
>> Active VMs in this list will be used by the I/O requests dispatching
>> tasklet ioreq_tasklet, whose callback function is ioreq_tasklet_handler()
>> in patch 0009. ioreq_tasklet_handler() currently handles the notification
>> interrupt from the hypervisor and dispatches I/O requests to each VMs.
> 
> So you need to somehow look through the whole list of devices for every
> I/O request?  That feels really really wrong, why don't you have that
> pointer in the first place?
> 
> Again, step back and describe what you need/desire and then think about
> how to best solve that.  Almost always, a list of objects that you have
> to iterate over all the time is not the way to do it...

Each VM has a shared buffer for I/O requests passing with the
hypervisor. Currently, the hypervisor doesn't indicate the VMs which has
pending I/O requests. So when kernel get the notification interrupt, it
search all VMs' shared buffer and dispatch the pending I/O requests.

The current I/O requests dispatching implementation uses one global
tasklet (be scheduled in the hypervisor notification interrupt), so it
needs to iterate all VMs to do the dispatching.

Each VM has a dedicated hypervisor notification interrupt vector might
be suited (a vector can be linked with a VM). The disadvantage is that
it might occupy many vectors.

Looking forward to more suggestions. Thanks very much.

> 
> Somedays I think we need an "here's how to do the things you really need
> to do in a driver" chapter in the Linux Device Driver's book..
That will be great. :)

Thanks
shuo

Re: [PATCH v2 07/17] virt: acrn: Introduce an ioctl to set vCPU registers state

[Liu, Shuo A]

Hi Greg,

On 9/3/2020 21:03, Greg Kroah-Hartman wrote:
> On Thu, Sep 03, 2020 at 08:41:51PM +0800, shuo.a@intel.com wrote:
>> From: Shuo Liu 
>>
>> A virtual CPU of User VM has different context due to the different
>> registers state. ACRN userspace needs to set the virtual CPU
>> registers state (e.g. giving a initial registers state to a virtual
>> BSP of a User VM).
>>
>> HSM provides an ioctl ACRN_IOCTL_SET_VCPU_REGS to do the virtual CPU
>> registers state setting. The ioctl passes the registers state from ACRN
>> userspace to the hypervisor directly.
>>
>> Signed-off-by: Shuo Liu 
>> Reviewed-by: Zhi Wang 
>> Reviewed-by: Reinette Chatre 
>> Cc: Zhi Wang 
>> Cc: Zhenyu Wang 
>> Cc: Yu Wang 
>> Cc: Reinette Chatre 
>> Cc: Greg Kroah-Hartman 
>> ---
>>  drivers/virt/acrn/hsm.c   | 14 +++
>>  drivers/virt/acrn/hypercall.h | 13 +++
>>  include/uapi/linux/acrn.h | 71 +++
>>  3 files changed, 98 insertions(+)
>>
>> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>> index 6ec6aa9053d3..13df76d0206e 100644
>> --- a/drivers/virt/acrn/hsm.c
>> +++ b/drivers/virt/acrn/hsm.c
>> @@ -12,6 +12,7 @@
>>  #define pr_fmt(fmt) "acrn: " fmt
>>  #define dev_fmt(fmt) "acrn: " fmt
>>  
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -49,6 +50,7 @@ static long acrn_dev_ioctl(struct file *filp, unsigned int 
>> cmd,
>>  {
>>  struct acrn_vm *vm = filp->private_data;
>>  struct acrn_vm_creation *vm_param;
>> +struct acrn_vcpu_regs *cpu_regs;
>>  int ret = 0;
>>  
>>  if (vm->vmid == ACRN_INVALID_VMID && cmd != ACRN_IOCTL_CREATE_VM) {
>> @@ -96,6 +98,18 @@ static long acrn_dev_ioctl(struct file *filp, unsigned 
>> int cmd,
>>  case ACRN_IOCTL_DESTROY_VM:
>>  ret = acrn_vm_destroy(vm);
>>  break;
>> +case ACRN_IOCTL_SET_VCPU_REGS:
>> +cpu_regs = memdup_user((void __user *)ioctl_param,
>> +   sizeof(struct acrn_vcpu_regs));
>> +if (IS_ERR(cpu_regs))
>> +return PTR_ERR(cpu_regs);
>> +
>> +ret = hcall_set_vcpu_regs(vm->vmid, virt_to_phys(cpu_regs));
> 
> No sanity checking of any arguments?

The HSM driver has limited VM status maintenance so it doesn't have full
ability to do the sanity checking.

> 
> Wow, fuzzers are going to have a fun time with your hypervisor, good
> luck!  :)

The hypervisor has some sanity checking. :)

Thanks
shuo

Re: [PATCH v2 06/17] virt: acrn: Introduce VM management interfaces

[Liu, Shuo A]

Hi Greg,

On 9/3/2020 21:02, Greg Kroah-Hartman wrote:
> On Thu, Sep 03, 2020 at 08:41:50PM +0800, shuo.a@intel.com wrote:
>> From: Shuo Liu 
>>
>> The VM management interfaces expose several VM operations to ACRN
>> userspace via ioctls. For example, creating VM, starting VM, destroying
>> VM and so on.
>>
>> The ACRN Hypervisor needs to exchange data with the ACRN userspace
>> during the VM operations. HSM provides VM operation ioctls to the ACRN
>> userspace and communicates with the ACRN Hypervisor for VM operations
>> via hypercalls.
>>
>> HSM maintains a list of User VM. Each User VM will be bound to an
>> existing file descriptor of /dev/acrn_hsm. The User VM will be
>> destroyed when the file descriptor is closed.
>>
>> Signed-off-by: Shuo Liu 
>> Reviewed-by: Zhi Wang 
>> Reviewed-by: Reinette Chatre 
>> Cc: Zhi Wang 
>> Cc: Zhenyu Wang 
>> Cc: Yu Wang 
>> Cc: Reinette Chatre 
>> Cc: Greg Kroah-Hartman 
>> ---
>>  drivers/virt/acrn/Makefile|  2 +-
>>  drivers/virt/acrn/acrn_drv.h  | 21 +-
>>  drivers/virt/acrn/hsm.c   | 62 +++-
>>  drivers/virt/acrn/hypercall.h | 78 +++
>>  drivers/virt/acrn/vm.c| 67 ++
>>  include/uapi/linux/acrn.h | 39 ++
>>  6 files changed, 266 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/virt/acrn/hypercall.h
>>  create mode 100644 drivers/virt/acrn/vm.c
>>
>> diff --git a/drivers/virt/acrn/Makefile b/drivers/virt/acrn/Makefile
>> index 6920ed798aaf..cf8b4ed5e74e 100644
>> --- a/drivers/virt/acrn/Makefile
>> +++ b/drivers/virt/acrn/Makefile
>> @@ -1,3 +1,3 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  obj-$(CONFIG_ACRN_HSM)  := acrn.o
>> -acrn-y := hsm.o
>> +acrn-y := hsm.o vm.o
>> diff --git a/drivers/virt/acrn/acrn_drv.h b/drivers/virt/acrn/acrn_drv.h
>> index 0b8e4fdc168a..043ae6840995 100644
>> --- a/drivers/virt/acrn/acrn_drv.h
>> +++ b/drivers/virt/acrn/acrn_drv.h
>> @@ -4,16 +4,35 @@
>>  #define __ACRN_HSM_DRV_H
>>  
>>  #include 
>> +#include 
>>  #include 
>>  
>> +#include "hypercall.h"
>> +
>>  #define ACRN_INVALID_VMID (0xU)
>>  
>> +#define ACRN_VM_FLAG_DESTROYED  0U
>> +extern struct list_head acrn_vm_list;
>> +extern rwlock_t acrn_vm_list_lock;
>>  /**
>>   * struct acrn_vm - Properties of ACRN User VM.
>> + * @dev:The struct device this VM belongs to
>> + * @list:   Entry within global list of all VMs
>>   * @vmid:   User VM ID
>> + * @vcpu_num:   Number of virtual CPUs in the VM
>> + * @flags:  Flags (ACRN_VM_FLAG_*) of the VM. This is VM flag management
>> + *  in HSM which is different from the _vm_creation.vm_flag.
>>   */
>>  struct acrn_vm {
>> -u16 vmid;
>> +struct device   *dev;
>> +struct list_headlist;
>> +u16 vmid;
>> +int vcpu_num;
>> +unsigned long   flags;
>>  };
>>  
>> +struct acrn_vm *acrn_vm_create(struct acrn_vm *vm,
>> +   struct acrn_vm_creation *vm_param);
>> +int acrn_vm_destroy(struct acrn_vm *vm);
>> +
>>  #endif /* __ACRN_HSM_DRV_H */
>> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>> index 549c7f8d6b5f..6ec6aa9053d3 100644
>> --- a/drivers/virt/acrn/hsm.c
>> +++ b/drivers/virt/acrn/hsm.c
>> @@ -10,6 +10,7 @@
>>   */
>>  
>>  #define pr_fmt(fmt) "acrn: " fmt
>> +#define dev_fmt(fmt) "acrn: " fmt
> 
> This should not be needed anywhere, what is wrong with the default
> prefix given to you by the dev_*() calls?

OK, the default prefix is enough. I will remove this define.

> 
>>  
>>  #include 
>>  #include 
>> @@ -21,6 +22,8 @@
>>  
>>  #include "acrn_drv.h"
>>  
>> +static struct device *dev;
> 
> Um, why?  This feels really odd...

Sorry, my foolish. :)
Will use 'static struct miscdevice acrn_dev' directly.

>> +
>>  /*
>>   * When /dev/acrn_hsm is opened, a 'struct acrn_vm' object is created to
>>   * represent a VM instance and continues to be associated with the opened 
>> file
>> @@ -36,6 +39,7 @@ static int acrn_dev_open(struct inode *inode, struct file 
>> *filp)
>>  return -ENOMEM;
>>  
>>  vm->vmid = ACRN_INVALID_VMID;
>> +vm->dev = dev;
>>  filp->private_data = vm;
>>  return 0;
>>  }
>> @@ -43,13 +47,68 @@ static int acrn_dev_open(struct inode *inode, struct 
>> file *filp)
>>  static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long ioctl_param)
>>  {
>> -return 0;
>> +struct acrn_vm *vm = filp->private_data;
>> +struct acrn_vm_creation *vm_param;
>> +int ret = 0;
>> +
>> +if (vm->vmid == ACRN_INVALID_VMID && cmd != ACRN_IOCTL_CREATE_VM) {
>> +dev_err(dev, "ioctl 0x%x: Invalid VM state!\n", cmd);
>> +return -EFAULT;
>> +}
>> +
>> +switch (cmd) {
>> +case ACRN_IOCTL_CREATE_VM:
>> +vm_param = memdup_user((void __user *)ioctl_param,
>> +   

Re: [PATCH v2 05/17] virt: acrn: Introduce ACRN HSM basic driver

[Liu, Shuo A]

On 9/3/2020 20:53, Greg Kroah-Hartman wrote:
> On Thu, Sep 03, 2020 at 08:41:49PM +0800, shuo.a@intel.com wrote:
>> From: Shuo Liu 
>>
>> ACRN Hypervisor Service Module (HSM) is a kernel module in Service VM
>> which communicates with ACRN userspace through ioctls and talks to ACRN
>> Hypervisor through hypercalls.
>>
>> Add a basic HSM driver which allows Service VM userspace to communicate
>> with ACRN. The following patches will add more ioctls, guest VM memory
>> mapping caching, I/O request processing, ioeventfd and irqfd into this
>> module. HSM exports a char device interface (/dev/acrn_hsm) to userspace.
>>
>> Signed-off-by: Shuo Liu 
>> Reviewed-by: Reinette Chatre 
>> Cc: Dave Hansen 
>> Cc: Zhi Wang 
>> Cc: Zhenyu Wang 
>> Cc: Yu Wang 
>> Cc: Reinette Chatre 
>> Cc: Greg Kroah-Hartman 
>> ---
>>  .../userspace-api/ioctl/ioctl-number.rst  |  1 +
>>  MAINTAINERS   |  2 +
>>  drivers/virt/Kconfig  |  2 +
>>  drivers/virt/Makefile |  1 +
>>  drivers/virt/acrn/Kconfig | 14 +++
>>  drivers/virt/acrn/Makefile|  3 +
>>  drivers/virt/acrn/acrn_drv.h  | 19 
>>  drivers/virt/acrn/hsm.c   | 98 +++
>>  include/uapi/linux/acrn.h | 17 
>>  9 files changed, 157 insertions(+)
>>  create mode 100644 drivers/virt/acrn/Kconfig
>>  create mode 100644 drivers/virt/acrn/Makefile
>>  create mode 100644 drivers/virt/acrn/acrn_drv.h
>>  create mode 100644 drivers/virt/acrn/hsm.c
>>  create mode 100644 include/uapi/linux/acrn.h
>>
>> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
>> b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> index 2a198838fca9..ac60efedb104 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -319,6 +319,7 @@ Code  Seq#Include File   
>> Comments
>>  0xA0  alllinux/sdp/sdp.h 
>> Industrial Device Project
>>   
>> 
>>  0xA1  0  linux/vtpm_proxy.h  TPM 
>> Emulator Proxy Driver
>> +0xA2  alluapi/linux/acrn.h   ACRN 
>> hypervisor
> 
> You don't have any ioctls in this patch, so why add this documentation
> here? 

This was left when i removed an api version ioctl from the v1 patch set.
Let me move it to a later patch.

>>  0xA3  80-8F  Port 
>> ACL  in development:
>>   
>> 
>>  0xA3  90-9F  linux/dtlk.h
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e0fea5e464b4..d4c1ef303c2d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -442,6 +442,8 @@ L:   acrn-...@lists.projectacrn.org
>>  S:  Supported
>>  W:  https://projectacrn.org
>>  F:  Documentation/virt/acrn/
>> +F:  drivers/virt/acrn/
>> +F:  include/uapi/linux/acrn.h
> 
> This uapi file is not used in this patch, please add it in a later
> patch.

OK.

> 
>> +static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
>> +   unsigned long ioctl_param)
>> +{
>> +return 0;
>> +}
> 
> As your ioctl does nothing, no need to include it here, add it in a
> later patch.

OK.

> 
>> +
>> +static int acrn_dev_release(struct inode *inode, struct file *filp)
>> +{
>> +struct acrn_vm *vm = filp->private_data;
>> +
>> +kfree(vm);
>> +return 0;
>> +}
>> +
>> +static const struct file_operations acrn_fops = {
>> +.owner  = THIS_MODULE,
>> +.open   = acrn_dev_open,
>> +.release= acrn_dev_release,
>> +.unlocked_ioctl = acrn_dev_ioctl,
>> +};
>> +
>> +static struct miscdevice acrn_dev = {
>> +.minor  = MISC_DYNAMIC_MINOR,
>> +.name   = "acrn_hsm",
>> +.fops   = _fops,
>> +};
>> +
>> +static int __init hsm_init(void)
>> +{
>> +int ret;
>> +
>> +if (x86_hyper_type != X86_HYPER_ACRN)
>> +return -ENODEV;
>> +
>> +if (!acrn_is_privileged_vm())
>> +return -EPERM;
>> +
>> +ret = misc_register(_dev);
>> +if (ret) {
>> +pr_err("Create misc dev failed!\n");
>> +return ret;
>> +}
>> +
>> +return 0;
> 
> Tiny tiny nit, these lines can be rewritten as:
>   if (ret)
>   pr_err("Create misc dev failed!\n");
> 
>   return ret;
> 
> :)

OK. Thanks.

> 
>> +}
>> +
>> +static void __exit hsm_exit(void)
>> +{
>> +misc_deregister(_dev);
>> +}
>> +module_init(hsm_init);
>> +module_exit(hsm_exit);
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("ACRN Hypervisor Service Module (HSM)");
>> diff --git 

[PATCH] xen/events: Fix interrupt lost during irq_disable and irq_enable

[Liu Shuo]

Here is a device has xen-pirq-MSI interrupt. Dom0 might lost interrupt
during driver irq_disable/irq_enable. Here is the scenario,
 1. irq_disable -> disable_dynirq -> mask_evtchn(irq channel)
 2. dev interrupt raised by HW and Xen mark its evtchn as pending
 3. irq_enable -> startup_pirq -> eoi_pirq ->
clear_evtchn(channel of irq) -> clear pending status
 4. consume_one_event process the irq event without pending bit assert
which result in interrupt lost once
 5. No HW interrupt raising anymore.

Now use enable_dynirq for enable_pirq of xen_pirq_chip to remove
eoi_pirq when irq_enable.

Signed-off-by: Liu Shuo <shuo.a@intel.com>
---
 drivers/xen/events/events_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index bae1f5d3..2d43118 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -574,7 +574,7 @@ static void shutdown_pirq(struct irq_data *data)
 
 static void enable_pirq(struct irq_data *data)
 {
-   startup_pirq(data);
+   enable_dynirq(data);
 }
 
 static void disable_pirq(struct irq_data *data)
-- 
1.9.4

[PATCH] xen/events: Fix interrupt lost during irq_disable and irq_enable

[Liu Shuo]

Here is a device has xen-pirq-MSI interrupt. Dom0 might lost interrupt
during driver irq_disable/irq_enable. Here is the scenario,
 1. irq_disable -> disable_dynirq -> mask_evtchn(irq channel)
 2. dev interrupt raised by HW and Xen mark its evtchn as pending
 3. irq_enable -> startup_pirq -> eoi_pirq ->
clear_evtchn(channel of irq) -> clear pending status
 4. consume_one_event process the irq event without pending bit assert
which result in interrupt lost once
 5. No HW interrupt raising anymore.

Now use enable_dynirq for enable_pirq of xen_pirq_chip to remove
eoi_pirq when irq_enable.

Signed-off-by: Liu Shuo 
---
 drivers/xen/events/events_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index bae1f5d3..2d43118 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -574,7 +574,7 @@ static void shutdown_pirq(struct irq_data *data)
 
 static void enable_pirq(struct irq_data *data)
 {
-   startup_pirq(data);
+   enable_dynirq(data);
 }
 
 static void disable_pirq(struct irq_data *data)
-- 
1.9.4

Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb

[Liu Shuo]

On Thu 12.Jan'17 at 17:33:38 +0100, Oliver Hartkopp wrote:

On 01/12/2017 02:01 PM, Eric Dumazet wrote:

On Thu, 2017-01-12 at 09:22 +0100, Oliver Hartkopp wrote:



But my main concern is:

The reason why can_rx_delete_receiver() was introduced was the need to
remove a huge number of receivers with can_rx_unregister().

When you call synchronize_rcu() after each receiver removal this would
potentially lead to a big performance issue when e.g. closing CAN_RAW
sockets with a high number of receivers.

So the idea was to remove/unlink the receiver hlist_del_rcu(>list)
and also kmem_cache_free(rcv_cache, r) by some rcu mechanism - so that
all elements are cleaned up by rcu at a later point.

Is it possible that the problems emerge due to hlist_del_rcu(>list)
and you accidently fix it with your introduced synchronize_rcu()?


I agree this patch does not fix the root cause.

The main problem seems that the sockets themselves are not RCU
protected.

If CAN uses RCU for delivery, then sockets should be freed only after
one RCU grace period.

On recent kernels, following patch could help :



Thanks Eric!

@Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your 
setup?

Sorry for late reply. I was OOO yesterday.
With Eric's hint, i just found his patch that "net: add SOCK_RCU_FREE
socket flag" in the latest kernel. With backporting this one plus Eric's
following patch, it fixs my failure.

Thanks Eric and Oliver!

Shuo


Best regards,
Oliver

Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb

[Liu Shuo]

On Thu 12.Jan'17 at 17:33:38 +0100, Oliver Hartkopp wrote:

On 01/12/2017 02:01 PM, Eric Dumazet wrote:

On Thu, 2017-01-12 at 09:22 +0100, Oliver Hartkopp wrote:



But my main concern is:

The reason why can_rx_delete_receiver() was introduced was the need to
remove a huge number of receivers with can_rx_unregister().

When you call synchronize_rcu() after each receiver removal this would
potentially lead to a big performance issue when e.g. closing CAN_RAW
sockets with a high number of receivers.

So the idea was to remove/unlink the receiver hlist_del_rcu(>list)
and also kmem_cache_free(rcv_cache, r) by some rcu mechanism - so that
all elements are cleaned up by rcu at a later point.

Is it possible that the problems emerge due to hlist_del_rcu(>list)
and you accidently fix it with your introduced synchronize_rcu()?


I agree this patch does not fix the root cause.

The main problem seems that the sockets themselves are not RCU
protected.

If CAN uses RCU for delivery, then sockets should be freed only after
one RCU grace period.

On recent kernels, following patch could help :



Thanks Eric!

@Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your 
setup?

Sorry for late reply. I was OOO yesterday.
With Eric's hint, i just found his patch that "net: add SOCK_RCU_FREE
socket flag" in the latest kernel. With backporting this one plus Eric's
following patch, it fixs my failure.

Thanks Eric and Oliver!

Shuo


Best regards,
Oliver

Re: [PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

On Fri 15.Jul'16 at  6:09:59 +0100, Al Viro wrote:

On Fri, Jul 15, 2016 at 11:18:41AM +0800, Liu Shuo wrote:


If there is no such thread (who operates the descriptor based on
guessing), i can think the changing is safe at the point. As the fd has
not been delivered to userspace. Am i right?



Expecting nice behaviour from userland code is something best avoided, really.


Got it! :)


All jokes aside, this other thread doesn't have to be malicious - just being
buggy would suffice.  Besides, you never know if something like userns won't
be dumped into the kernel, making your ioctl accessible to genuinely
malicious code.

The only sane approach is to treat descriptor tables as shared data structures
and postpone the insertion of struct file reference into descriptor table
until you are past all failure exits.  Including the ones related to copying
to userland - e.g. pipe(2) creates a pipe, sets up two struct file associated
with it, reserves two descriptors, copies them into userland array and only if
everything has succeeded proceeds to fd_install().  In your case passing the
descriptor to userland is not an issue (return value of ioctl(2) goes via
register and that can't fail), so the last failure exit is that after failed
attempt to create debugfs stuff.  We have to reserve the descriptor before
that (it's used as a part of debugfs directory name), so anon_inode_getfd()
is not an option - it combines reserving descriptor with fd_install().
Such situations are exactly the reason why anon_inode_getfile() is there;
anon_inode_getfd() is usable only when it is the very last thing we do
before returning the descriptor to userland.

Really appreciate your exhaustive explanations. Thanks.


FWIW, original code was not unreasonable - it simply treated debugfs stuff as
optional and ignored those failures.  That way anon_inode_getfd() is fine -
there's no failure exits after it.  If we want to fail when debugfs had
been enabled and we'd failed to populate it, we need to use the real primitives
behind anon_inode_getfd(), though.

So, my firstly immature idea returns back.
Is the fd used by debugfs name very useful? Do we really need it here?
Alternative approach is putting the fd into the debugfs, then the code
will be clean here, as we can put anon_inode_getfd at very last of the
ioctl.

Re: [PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

On Fri 15.Jul'16 at  6:09:59 +0100, Al Viro wrote:

On Fri, Jul 15, 2016 at 11:18:41AM +0800, Liu Shuo wrote:


If there is no such thread (who operates the descriptor based on
guessing), i can think the changing is safe at the point. As the fd has
not been delivered to userspace. Am i right?



Expecting nice behaviour from userland code is something best avoided, really.


Got it! :)


All jokes aside, this other thread doesn't have to be malicious - just being
buggy would suffice.  Besides, you never know if something like userns won't
be dumped into the kernel, making your ioctl accessible to genuinely
malicious code.

The only sane approach is to treat descriptor tables as shared data structures
and postpone the insertion of struct file reference into descriptor table
until you are past all failure exits.  Including the ones related to copying
to userland - e.g. pipe(2) creates a pipe, sets up two struct file associated
with it, reserves two descriptors, copies them into userland array and only if
everything has succeeded proceeds to fd_install().  In your case passing the
descriptor to userland is not an issue (return value of ioctl(2) goes via
register and that can't fail), so the last failure exit is that after failed
attempt to create debugfs stuff.  We have to reserve the descriptor before
that (it's used as a part of debugfs directory name), so anon_inode_getfd()
is not an option - it combines reserving descriptor with fd_install().
Such situations are exactly the reason why anon_inode_getfile() is there;
anon_inode_getfd() is usable only when it is the very last thing we do
before returning the descriptor to userland.

Really appreciate your exhaustive explanations. Thanks.


FWIW, original code was not unreasonable - it simply treated debugfs stuff as
optional and ignored those failures.  That way anon_inode_getfd() is fine -
there's no failure exits after it.  If we want to fail when debugfs had
been enabled and we'd failed to populate it, we need to use the real primitives
behind anon_inode_getfd(), though.

So, my firstly immature idea returns back.
Is the fd used by debugfs name very useful? Do we really need it here?
Alternative approach is putting the fd into the debugfs, then the code
will be clean here, as we can put anon_inode_getfd at very last of the
ioctl.

Re: [PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

On Fri 15.Jul'16 at  3:26:03 +0100, Al Viro wrote:

On Fri, Jul 15, 2016 at 10:22:04AM +0800, Liu Shuo wrote:

> You have no warranty whatsoever that descriptor table has not been changed
> by that point.  You should *NEVER* use sys_close() on failure exit paths
Could you please elaborate why we're not sure descriptor table's changing at 
the point?


Because that could be called by one thread while another (having guessed the
descriptor you are about to get) does close()/dup2()/etc.

If there is no such thread (who operates the descriptor based on
guessing), i can think the changing is safe at the point. As the fd has
not been delivered to userspace. Am i right?

Re: [PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

On Fri 15.Jul'16 at  3:26:03 +0100, Al Viro wrote:

On Fri, Jul 15, 2016 at 10:22:04AM +0800, Liu Shuo wrote:

> You have no warranty whatsoever that descriptor table has not been changed
> by that point.  You should *NEVER* use sys_close() on failure exit paths
Could you please elaborate why we're not sure descriptor table's changing at 
the point?


Because that could be called by one thread while another (having guessed the
descriptor you are about to get) does close()/dup2()/etc.

If there is no such thread (who operates the descriptor based on
guessing), i can think the changing is safe at the point. As the fd has
not been delivered to userspace. Am i right?

Re: [PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

On Thu 14.Jul'16 at 17:46:47 +0100, Al Viro wrote:

On Tue, Jul 12, 2016 at 12:24:39PM +0200, Paolo Bonzini wrote:


On 12/07/2016 11:38, Liu Shuo wrote:
> The failure of create debugfs of VM will return directly without release
> the anon file. It will leak memory and file descriptors, even through
> be not serious.
>
> Signed-off-by: Liu Shuo <shuo.a@intel.com>


This is broken.


> @@ -3067,6 +3068,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>
>if (kvm_create_vm_debugfs(kvm, r) < 0) {
>kvm_put_kvm(kvm);
> +  sys_close(r);


You have no warranty whatsoever that descriptor table has not been changed
by that point.  You should *NEVER* use sys_close() on failure exit paths

Could you please elaborate why we're not sure descriptor table's changing at 
the point?

like that.  Moreover, this kvm_put_kvm() becomes a double-put, since
closing the damn file will drop that reference to kvm.

You're right. Thanks for the correction on this point.



>return -ENOMEM;
>}
>
>

Thanks, applied to kvm/master.


Please, revert.  anon_inode_getfd() should be used only when there's no
possible failures past its call.  What you need in this case (due to fucking
ugly API - decriptor number used as part of debugfs pathname) is something
like this (against mainline):

[kvm] don't use anon_inode_getfd() before possible failures

Once anon_inode_getfd() has succeeded, it's impossible to undo in a clean
way and no, sys_close() is not usable in such cases.  Use anon_inode_getfile()
and get_unused_fd_flags() to get struct file and descriptor and do *not*
install the file into descriptor table until after the last possible failure
exit.

Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>

Thanks. It's better.

---
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 48bd520..7f82c6c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3048,6 +3048,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
{
int r;
struct kvm *kvm;
+   struct file *file;

kvm = kvm_create_vm(type);
if (IS_ERR(kvm))
@@ -3059,17 +3060,25 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
return r;
}
#endif
-   r = anon_inode_getfd("kvm-vm", _vm_fops, kvm, O_RDWR | O_CLOEXEC);
+   r = get_unused_fd_flags(O_CLOEXEC);
if (r < 0) {
kvm_put_kvm(kvm);
return r;
}
+   file = anon_inode_getfile("kvm-vm", _vm_fops, kvm, O_RDWR);
+   if (IS_ERR(file)) {
+   put_unused_fd(r);
+   kvm_put_kvm(kvm);
+   return PTR_ERR(file);
+   }

if (kvm_create_vm_debugfs(kvm, r) < 0) {
-   kvm_put_kvm(kvm);
+   put_unused_fd(r);
+   fput(file);
return -ENOMEM;
}

+   fd_install(r, file);
return r;
}

Re: [PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

On Thu 14.Jul'16 at 17:46:47 +0100, Al Viro wrote:

On Tue, Jul 12, 2016 at 12:24:39PM +0200, Paolo Bonzini wrote:


On 12/07/2016 11:38, Liu Shuo wrote:
> The failure of create debugfs of VM will return directly without release
> the anon file. It will leak memory and file descriptors, even through
> be not serious.
>
> Signed-off-by: Liu Shuo 


This is broken.


> @@ -3067,6 +3068,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>
>if (kvm_create_vm_debugfs(kvm, r) < 0) {
>kvm_put_kvm(kvm);
> +  sys_close(r);


You have no warranty whatsoever that descriptor table has not been changed
by that point.  You should *NEVER* use sys_close() on failure exit paths

Could you please elaborate why we're not sure descriptor table's changing at 
the point?

like that.  Moreover, this kvm_put_kvm() becomes a double-put, since
closing the damn file will drop that reference to kvm.

You're right. Thanks for the correction on this point.



>return -ENOMEM;
>}
>
>

Thanks, applied to kvm/master.


Please, revert.  anon_inode_getfd() should be used only when there's no
possible failures past its call.  What you need in this case (due to fucking
ugly API - decriptor number used as part of debugfs pathname) is something
like this (against mainline):

[kvm] don't use anon_inode_getfd() before possible failures

Once anon_inode_getfd() has succeeded, it's impossible to undo in a clean
way and no, sys_close() is not usable in such cases.  Use anon_inode_getfile()
and get_unused_fd_flags() to get struct file and descriptor and do *not*
install the file into descriptor table until after the last possible failure
exit.

Signed-off-by: Al Viro 

Thanks. It's better.

---
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 48bd520..7f82c6c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3048,6 +3048,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
{
int r;
struct kvm *kvm;
+   struct file *file;

kvm = kvm_create_vm(type);
if (IS_ERR(kvm))
@@ -3059,17 +3060,25 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
return r;
}
#endif
-   r = anon_inode_getfd("kvm-vm", _vm_fops, kvm, O_RDWR | O_CLOEXEC);
+   r = get_unused_fd_flags(O_CLOEXEC);
if (r < 0) {
kvm_put_kvm(kvm);
return r;
}
+   file = anon_inode_getfile("kvm-vm", _vm_fops, kvm, O_RDWR);
+   if (IS_ERR(file)) {
+   put_unused_fd(r);
+   kvm_put_kvm(kvm);
+   return PTR_ERR(file);
+   }

if (kvm_create_vm_debugfs(kvm, r) < 0) {
-   kvm_put_kvm(kvm);
+   put_unused_fd(r);
+   fput(file);
return -ENOMEM;
}

+   fd_install(r, file);
return r;
}

[PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

The failure of create debugfs of VM will return directly without release
the anon file. It will leak memory and file descriptors, even through
be not serious.

Signed-off-by: Liu Shuo <shuo.a@intel.com>
---
 virt/kvm/kvm_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 48bd520..8322154 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -3067,6 +3068,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 
if (kvm_create_vm_debugfs(kvm, r) < 0) {
kvm_put_kvm(kvm);
+   sys_close(r);
return -ENOMEM;
}
 
-- 
1.9.4

[PATCH] KVM: release anon file in failure path of vm creation

[Liu Shuo]

The failure of create debugfs of VM will return directly without release
the anon file. It will leak memory and file descriptors, even through
be not serious.

Signed-off-by: Liu Shuo 
---
 virt/kvm/kvm_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 48bd520..8322154 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -3067,6 +3068,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 
if (kvm_create_vm_debugfs(kvm, r) < 0) {
kvm_put_kvm(kvm);
+   sys_close(r);
return -ENOMEM;
}
 
-- 
1.9.4

[PATCH 2/2] pstore: fix memory leak when decompress using big_oops_buf

[Liu Shuo]

From: Liu ShuoX 

After sucessful decompressing, the buffer which pointed by 'buf' will be
lost as 'buf' is overwrite by 'big_oops_buf' and will never be freed. 


Signed-off-by: Liu ShuoX 
---
fs/pstore/platform.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 78c3c20..46d269e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -497,6 +497,7 @@ void pstore_get_records(int quiet)
big_oops_buf_sz);

if (unzipped_len > 0) {
+   kfree(buf);
buf = big_oops_buf;
size = unzipped_len;
compressed = false;
--
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] pstore: fix buffer overflow while write offset equal to buffer size

[Liu Shuo]

From: Liu ShuoX 

In case new offset is equal to prz->buffer_size, it won't wrap at this
time and will return old(overflow) value next time.

Signed-off-by: Liu ShuoX 
---
fs/pstore/ram_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index de272d4..ff7e3d4 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -54,7 +54,7 @@ static size_t buffer_start_add_atomic(struct 
persistent_ram_zone *prz, size_t a)
do {
old = atomic_read(>buffer->start);
new = old + a;
-   while (unlikely(new > prz->buffer_size))
+   while (unlikely(new >= prz->buffer_size))
new -= prz->buffer_size;
} while (atomic_cmpxchg(>buffer->start, old, new) != old);

@@ -91,7 +91,7 @@ static size_t buffer_start_add_locked(struct 
persistent_ram_zone *prz, size_t a)

old = atomic_read(>buffer->start);
new = old + a;
-   while (unlikely(new > prz->buffer_size))
+   while (unlikely(new >= prz->buffer_size))
new -= prz->buffer_size;
atomic_set(>buffer->start, new);

--
1.8.3.2



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] pstore: fix buffer overflow while write offset equal to buffer size

[Liu Shuo]

From: Liu ShuoX shuox@intel.com

In case new offset is equal to prz-buffer_size, it won't wrap at this
time and will return old(overflow) value next time.

Signed-off-by: Liu ShuoX shuox@intel.com
---
fs/pstore/ram_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index de272d4..ff7e3d4 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -54,7 +54,7 @@ static size_t buffer_start_add_atomic(struct 
persistent_ram_zone *prz, size_t a)
do {
old = atomic_read(prz-buffer-start);
new = old + a;
-   while (unlikely(new  prz-buffer_size))
+   while (unlikely(new = prz-buffer_size))
new -= prz-buffer_size;
} while (atomic_cmpxchg(prz-buffer-start, old, new) != old);

@@ -91,7 +91,7 @@ static size_t buffer_start_add_locked(struct 
persistent_ram_zone *prz, size_t a)

old = atomic_read(prz-buffer-start);
new = old + a;
-   while (unlikely(new  prz-buffer_size))
+   while (unlikely(new = prz-buffer_size))
new -= prz-buffer_size;
atomic_set(prz-buffer-start, new);

--
1.8.3.2



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] pstore: fix memory leak when decompress using big_oops_buf

[Liu Shuo]

From: Liu ShuoX shuox@intel.com

After sucessful decompressing, the buffer which pointed by 'buf' will be
lost as 'buf' is overwrite by 'big_oops_buf' and will never be freed. 


Signed-off-by: Liu ShuoX shuox@intel.com
---
fs/pstore/platform.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 78c3c20..46d269e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -497,6 +497,7 @@ void pstore_get_records(int quiet)
big_oops_buf_sz);

if (unzipped_len  0) {
+   kfree(buf);
buf = big_oops_buf;
size = unzipped_len;
compressed = false;
--
1.8.3.2

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] pstore: clarify clearing of _read_cnt in ramoops_context

[Liu Shuo]

On Fri  7.Mar'14 at 16:23:29 -0800, Kees Cook wrote:

On Fri, Mar 7, 2014 at 1:25 PM, Andrew Morton  wrote:

On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX  wrote:



ftrace_read_cnt need to be reset in open to support mutli times
getting the records.

Signed-off-by: Liu ShuoX 
---
  fs/pstore/ram.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
...


The dump_read_cnt changes appear to be unrelated to the actual bugfix?


I recommended this change since it is being cleared during open, and
this zeroing was confusing.


If so, please send this along as a separate patch.  With a full
changelog - this one doesn't explain the dump_read_cnt changes at all.


Liu, I would recommend a changelog along the lines of:

Clarify which variables need to be cleared during pstore_open. Added
missed ftrace_read_cnt clearing and removed duplicate clearing in
ramoops_probe.

-Kees

Thanks. It's more clearly. I copied some of your comments into changelog. :)
Re-sent the patch with the new changelog. The subject also changed.


From: Liu ShuoX 

*_read_cnt in ramoops_context need to be cleared during pstore ->open
to support mutli times getting the records. The patch added missed
ftrace_read_cnt clearing and removed duplicate clearing in ramoops_probe.

Signed-off-by: Liu ShuoX 
---
fs/pstore/ram.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fa8cef2..9fe5b13 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -86,6 +86,7 @@ struct ramoops_context {
struct persistent_ram_ecc_info ecc_info;
unsigned int max_dump_cnt;
unsigned int dump_write_cnt;
+   /* _read_cnt need clear on ramoops_pstore_open */
unsigned int dump_read_cnt;
unsigned int console_read_cnt;
unsigned int ftrace_read_cnt;
@@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)

cxt->dump_read_cnt = 0;
cxt->console_read_cnt = 0;
+   cxt->ftrace_read_cnt = 0;
return 0;
}

@@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);

-   cxt->dump_read_cnt = 0;
cxt->size = pdata->mem_size;
cxt->phys_addr = pdata->mem_address;
cxt->record_size = pdata->record_size;
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] pstore: clarify clearing of _read_cnt in ramoops_context

[Liu Shuo]

On Fri  7.Mar'14 at 16:23:29 -0800, Kees Cook wrote:

On Fri, Mar 7, 2014 at 1:25 PM, Andrew Morton a...@linux-foundation.org wrote:

On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX shuox@intel.com wrote:



ftrace_read_cnt need to be reset in open to support mutli times
getting the records.

Signed-off-by: Liu ShuoX shuox@intel.com
---
  fs/pstore/ram.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
...


The dump_read_cnt changes appear to be unrelated to the actual bugfix?


I recommended this change since it is being cleared during open, and
this zeroing was confusing.


If so, please send this along as a separate patch.  With a full
changelog - this one doesn't explain the dump_read_cnt changes at all.


Liu, I would recommend a changelog along the lines of:

Clarify which variables need to be cleared during pstore_open. Added
missed ftrace_read_cnt clearing and removed duplicate clearing in
ramoops_probe.

-Kees

Thanks. It's more clearly. I copied some of your comments into changelog. :)
Re-sent the patch with the new changelog. The subject also changed.


From: Liu ShuoX shuox@intel.com

*_read_cnt in ramoops_context need to be cleared during pstore -open
to support mutli times getting the records. The patch added missed
ftrace_read_cnt clearing and removed duplicate clearing in ramoops_probe.

Signed-off-by: Liu ShuoX shuox@intel.com
---
fs/pstore/ram.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fa8cef2..9fe5b13 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -86,6 +86,7 @@ struct ramoops_context {
struct persistent_ram_ecc_info ecc_info;
unsigned int max_dump_cnt;
unsigned int dump_write_cnt;
+   /* _read_cnt need clear on ramoops_pstore_open */
unsigned int dump_read_cnt;
unsigned int console_read_cnt;
unsigned int ftrace_read_cnt;
@@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)

cxt-dump_read_cnt = 0;
cxt-console_read_cnt = 0;
+   cxt-ftrace_read_cnt = 0;
return 0;
}

@@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
if (pdata-ftrace_size  !is_power_of_2(pdata-ftrace_size))
pdata-ftrace_size = rounddown_pow_of_two(pdata-ftrace_size);

-   cxt-dump_read_cnt = 0;
cxt-size = pdata-mem_size;
cxt-phys_addr = pdata-mem_address;
cxt-record_size = pdata-record_size;
--
1.8.3.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/