Skip to content

Commit

Permalink
vsd3 task
Browse files Browse the repository at this point in the history
  • Loading branch information
Aspirisha committed Apr 27, 2016
1 parent 930db73 commit 141eae5
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 16 deletions.
20 changes: 15 additions & 5 deletions tasks/vsd3/vsd_device/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,22 +76,30 @@ static ssize_t vsd_dev_write(char *src, size_t src_size, size_t offset) {
return src_size;
}

static void vsd_dev_set_size(size_t size)
static ssize_t vsd_dev_set_size(size_t size)
{
// TODO implement command
// if size > VSD size then return -EINVAL to vsd_driver
// Don't forget to wake vsd_driver up using tasklet.
// Ensure that woken up task observes your writes
// to shared memory (hwregs).
// If new size is > current size then return -EINVAL.


if (size > dev.hwregs->dev_size)
return -EINVAL;
dev.hwregs->dev_size = size;
dev.buf_size = size;

return size;
}

static void vsd_dev_cmd_rw_after(ssize_t ret)
static void vsd_dev_cmd_rws_after(ssize_t ret)
{
struct tasklet_struct *comp_tasklet =
(struct tasklet_struct*)dev.hwregs->tasklet_vaddr;
dev.hwregs->result = ret < 0 ? (int32_t)ret : 0;
dev.hwregs->dma_size = (uint64_t)ret;
dev.hwregs->dma_size = (uint64_t)ret; // yep, this is useless for set_size, but who cares

This comment has been minimized.

Copy link
@eabatalov

eabatalov Apr 29, 2016

Collaborator

X-D

wmb();
dev.hwregs->cmd = VSD_CMD_NONE;
if (comp_tasklet) {
Expand All @@ -111,19 +119,21 @@ static int vsd_dev_cmd_poll_kthread_func(void *data)
(size_t)dev.hwregs->dma_size,
(size_t)dev.hwregs->dev_offset
);
vsd_dev_cmd_rw_after(ret);
vsd_dev_cmd_rws_after(ret);
break;
case VSD_CMD_WRITE:
ret = vsd_dev_write(
phys_to_virt((phys_addr_t)dev.hwregs->dma_paddr),
(size_t)dev.hwregs->dma_size,
(size_t)dev.hwregs->dev_offset
);
vsd_dev_cmd_rw_after(ret);
vsd_dev_cmd_rws_after(ret);
break;
case VSD_CMD_SET_SIZE:
// TODO call vsd_dev_set_size
// with right arguments
ret = vsd_dev_set_size((size_t)dev.hwregs->dev_offset);
vsd_dev_cmd_rws_after(ret);
break;
}

Expand Down
79 changes: 68 additions & 11 deletions tasks/vsd3/vsd_driver/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ typedef struct vsd_dev {
} vsd_dev_t;
static vsd_dev_t *vsd_dev;

#define LOCAL_DEBUG 0
#define LOCAL_DEBUG 1
static void print_vsd_dev_hw_regs(vsd_dev_t *vsd_dev)
{
if (!LOCAL_DEBUG)
Expand Down Expand Up @@ -68,6 +68,7 @@ static void vsd_dev_dma_op_complete_tsk_func(unsigned long unused)
{
(void)unused;
// TODO wakeup task waiting for completion of VSD cmd
wake_up(&vsd_dev->dma_op_compete_wq);
}

static ssize_t vsd_dev_read(struct file *filp,
Expand All @@ -76,14 +77,20 @@ static ssize_t vsd_dev_read(struct file *filp,
ssize_t ret = 0;
void *kdma_buf = NULL;

mutex_lock(&vsd_dev->dev_ops_serialization_mutex);
print_vsd_dev_hw_regs(vsd_dev);

/*

This comment has been minimized.

Copy link
@eabatalov

eabatalov Apr 29, 2016

Collaborator

You don't need this until... you wait_event_interruptible. But this is not enough in this case.
"Anyway who cares" :)

if (vsd_dev->hwregs->cmd != VSD_CMD_NONE) {
ret = -EBUSY;
goto exit;
}
}*/

// TODO check not to alloc too much DMA memory (easy DDOS)

if (read_size > vsd_dev->hwregs->dev_size) {
read_size = vsd_dev->hwregs->dev_size;
}

kdma_buf = kzalloc(read_size, GFP_KERNEL);
if (!kdma_buf) {
ret = -ENOMEM;
Expand All @@ -104,10 +111,21 @@ static ssize_t vsd_dev_read(struct file *filp,
* wait_event call to wait_event_interruptible call.
* Describe sequence of events (step by step) that lead to
* write to freed kernel buffer in vsd_* kernel code.
* 1. TODO
* 2. TODO
* 3. TODO
* ...
*
* Consider device size actually changes if we use SET_SIZE function
* i.e. device literally frees memory (for example) when we shrinke the size and allocates new
* as far as it fits into it's available size (2 pages).
* Now consider the following situation
* 1. user decided to change vsd size; we wait in vsd_ioctl_set_size. Meanwhile the state
* of device is being changed (allocation + sizes reassignment)
* 2. the signal awakens us, not condition; device is still in inconsistent state, but it
* turns out it has already increased own size (probably, that's stupid, but still)
* 3. by chance ret = vsd_dev->hwregs->result; is still 0 (why not, if last operation was
* successful)
* 4. we happily exit vsd_ioctl_set_size
* 5. we enter vsd_dev_write with a big amount of data to write, but since we "enlarged"
* device, it should be fine
* 6. vsd_dev_write in vsd_device/module.c writes too much data and fails

This comment has been minimized.

Copy link
@eabatalov

eabatalov Apr 29, 2016

Collaborator

You lock mutex_lock(&vsd_dev->dev_ops_serialization_mutex); in each vsd driver syscall. Nobody can change VSD size and write to it simultaneously. The right answer is:

  1. handling write call
  2. waiting in wait_event_interruptible()
  3. signal is delivered to waiting thread
  4. thread is woken up on vsd_dev->dma_op_compete_wq
  5. thread does
    exit_free_dma:
    kfree(kdma_buf);
  6. vsd_device writes to freed kdma_buf
*/
wait_event(vsd_dev->dma_op_compete_wq,
vsd_dev->hwregs->cmd == VSD_CMD_NONE);
Expand All @@ -128,7 +146,7 @@ static ssize_t vsd_dev_read(struct file *filp,
kfree(kdma_buf);
exit:
print_vsd_dev_hw_regs(vsd_dev);

mutex_unlock(&vsd_dev->dev_ops_serialization_mutex);
return ret;
}

Expand All @@ -138,13 +156,18 @@ static ssize_t vsd_dev_write(struct file *filp,
ssize_t ret = 0;
void *kdma_buf = NULL;

mutex_lock(&vsd_dev->dev_ops_serialization_mutex);
print_vsd_dev_hw_regs(vsd_dev);
/*
if (vsd_dev->hwregs->cmd != VSD_CMD_NONE) {
ret = -EBUSY;
goto exit;
}
}*/

// TODO check not to alloc too much DMA memory (easy DDOS)
if (write_size > vsd_dev->hwregs->dev_size) {
write_size = vsd_dev->hwregs->dev_size;
}
kdma_buf = kzalloc(write_size, GFP_KERNEL);
if (!kdma_buf) {
ret = -ENOMEM;
Expand Down Expand Up @@ -179,7 +202,7 @@ static ssize_t vsd_dev_write(struct file *filp,
kfree(kdma_buf);
exit:
print_vsd_dev_hw_regs(vsd_dev);

mutex_unlock(&vsd_dev->dev_ops_serialization_mutex);
return ret;
}

Expand Down Expand Up @@ -223,8 +246,42 @@ static long vsd_ioctl_get_size(vsd_ioctl_get_size_arg_t __user *uarg)

static long vsd_ioctl_set_size(vsd_ioctl_set_size_arg_t __user *uarg)
{
ssize_t ret = 0;
vsd_ioctl_set_size_arg_t arg;

// TODO implement
return 0;
mutex_lock(&vsd_dev->dev_ops_serialization_mutex);
print_vsd_dev_hw_regs(vsd_dev);
/*if (vsd_dev->hwregs->cmd != VSD_CMD_NONE) {
ret = -EBUSY;
goto exit;
}*/

if (copy_from_user(&arg, uarg, sizeof(arg))) {
ret = -EFAULT;
goto exit;
}

vsd_dev->hwregs->result = 0;
vsd_dev->hwregs->tasklet_vaddr =
(uint64_t)&vsd_dev->dma_op_complete_tsk;
vsd_dev->hwregs->dma_paddr = 0; // unused
vsd_dev->hwregs->dma_size = 0;
vsd_dev->hwregs->dev_offset = arg.size; // unused
wmb();
vsd_dev->hwregs->cmd = VSD_CMD_SET_SIZE;

wait_event(vsd_dev->dma_op_compete_wq,
vsd_dev->hwregs->cmd == VSD_CMD_NONE);

ret = vsd_dev->hwregs->result;
if (ret)

This comment has been minimized.

Copy link
@Aspirisha

Aspirisha Apr 27, 2016

Author Owner

Плачевный результат copy paste. 278-279 являются бесполезными строками.

goto exit;

exit:
print_vsd_dev_hw_regs(vsd_dev);
mutex_unlock(&vsd_dev->dev_ops_serialization_mutex);
return ret;
}

static long vsd_dev_ioctl(struct file *filp, unsigned int cmd,
Expand Down

0 comments on commit 141eae5

Please sign in to comment.