Hi.
I just added support for user space buffers in kfifo. I found useful
__kfifo_get_user to copy data to a user buffer in a read call. I didn't
like the idea of having an extra buffer.
* Is it ok to add this support?
* Am I missing something?
I expect to fill the fifo using __kfifo_put in interrupt context.
If I understand correctly, I won't need extra locking if I have a
single interrupt handler filling this buffer and all the readers use a
semaphore.
This is how I'm using it now:
static ssize_t device_read(struct file *filep, char __user *buffer,
size_t length, loff_t * offset)
{
struct sarpic_dev *dev = filep->private_data;
int ret_val;
if (down_interruptible (&dev->sem))
return -ERESTARTSYS;
while (__kfifo_len(dev->fifo) == 0) {
up(&dev->sem);
if (filep->f_flags & O_NONBLOCK)
return -EAGAIN;
if (wait_event_interruptible(dev->readq, (__kfifo_len(dev->fifo) != 0)))
return -ERESTARTSYS;
if (down_interruptible(&dev->sem))
return -ERESTARTSYS;
}
ret_val = __kfifo_get_user(dev->fifo, buffer, length);
up(&dev->sem);
return ret_val;
}
static ssize_t
device_write(struct file *filep,
const char __user *buffer, size_t length, loff_t *offset)
{
struct sarpic_dev *dev = filep->private_data;
int ret_val;
if (down_interruptible (&dev->sem))
return -ERESTARTSYS;
ret_val = __kfifo_put_user(dev->fifo, buffer, length);
up(&dev->sem);
wake_up_interruptible(&dev->readq);
return ret_val;
}
Regards,
Nelson.-
Signed-off-by: Nelson Castillo <[email protected]>
---
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 404f446..4568285 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -41,8 +41,13 @@ extern struct kfifo *kfifo_alloc(unsigned int size, gfp_t gfp_mask,
extern void kfifo_free(struct kfifo *fifo);
extern unsigned int __kfifo_put(struct kfifo *fifo,
unsigned char *buffer, unsigned int len);
+extern int __kfifo_put_user(struct kfifo *fifo,
+ const unsigned char __user *buffer,
+ unsigned int len);
extern unsigned int __kfifo_get(struct kfifo *fifo,
unsigned char *buffer, unsigned int len);
+extern int __kfifo_get_user(struct kfifo *fifo, unsigned char __user *buffer,
+ unsigned int len);
/**
* __kfifo_reset - removes the entire FIFO contents, no locking version
@@ -94,6 +99,32 @@ static inline unsigned int kfifo_put(struct kfifo *fifo,
}
/**
+ * kfifo_put_user - puts some user data into the FIFO
+ * @fifo: the fifo to be used.
+ * @buffer: the user-space data to be added.
+ * @len: the length of the data to be added.
+ *
+ * This function copies at most @len bytes from the @buffer into
+ * the FIFO depending on the free space, and returns the number of
+ * bytes copied.
+ */
+static inline int kfifo_put_user(struct kfifo *fifo,
+ const unsigned char __user *buffer,
+ unsigned int len)
+{
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(fifo->lock, flags);
+
+ ret = __kfifo_put_user(fifo, buffer, len);
+
+ spin_unlock_irqrestore(fifo->lock, flags);
+
+ return ret;
+}
+
+/**
* kfifo_get - gets some data from the FIFO
* @fifo: the fifo to be used.
* @buffer: where the data must be copied.
@@ -125,6 +156,34 @@ static inline unsigned int kfifo_get(struct kfifo *fifo,
}
/**
+ * kfifo_get_user - gets some data from the FIFO an user buffer
+ * @fifo: the fifo to be used.
+ * @buffer: user buffer where the data must be copied
+ * @len: the size of the destination buffer.
+ *
+ * This function copies at most @len bytes from the FIFO into the
+ * @buffer and returns the number of copied bytes.
+ */
+
+static inline int kfifo_get_user(struct kfifo *fifo,
+ unsigned char __user *buffer, unsigned int len)
+{
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(fifo->lock, flags);
+
+ ret = __kfifo_get_user(fifo, buffer, len);
+
+ if (fifo->in == fifo->out)
+ fifo->in = fifo->out = 0;
+
+ spin_unlock_irqrestore(fifo->lock, flags);
+
+ return ret;
+}
+
+/**
* __kfifo_len - returns the number of bytes available in the FIFO, no locking version
* @fifo: the fifo to be used.
*/
diff --git a/kernel/kfifo.c b/kernel/kfifo.c
index cee4191..e95ab37 100644
--- a/kernel/kfifo.c
+++ b/kernel/kfifo.c
@@ -23,6 +23,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/err.h>
+#include <asm/uaccess.h>
#include <linux/kfifo.h>
/**
@@ -150,6 +151,59 @@ unsigned int __kfifo_put(struct kfifo *fifo,
EXPORT_SYMBOL(__kfifo_put);
/**
+ * __kfifo_put_user - puts some data into the FIFO, from userspace
+ * no locking version
+ * @fifo: the fifo to be used.
+ * @buffer: the user space data to be added.
+ * @len: the length of the data to be added.
+ *
+ * This function copies at most @len bytes from the @buffer into
+ * the FIFO depending on the free space, and returns the number of
+ * bytes copied.
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these functions.
+ */
+int __kfifo_put_user(struct kfifo *fifo, const unsigned char __user *buffer,
+ unsigned int len)
+{
+ unsigned int l;
+
+ len = min(len, fifo->size - fifo->in + fifo->out);
+
+ /*
+ * Ensure that we sample the fifo->out index -before- we
+ * start putting bytes into the kfifo.
+ */
+
+ smp_mb();
+
+ /* first put the data starting from fifo->in to buffer end */
+ l = min(len, fifo->size - (fifo->in & (fifo->size - 1)));
+
+ if(copy_from_user(fifo->buffer + (fifo->in & (fifo->size - 1)),
+ buffer, l))
+ return -EFAULT;
+
+
+ /* then put the rest (if any) at the beginning of the buffer */
+ if (copy_from_user(fifo->buffer, buffer + l, len - l))
+ return -EFAULT;
+
+ /*
+ * Ensure that we add the bytes to the kfifo -before-
+ * we update the fifo->in index.
+ */
+
+ smp_wmb();
+
+ fifo->in += len;
+
+ return (int)len;
+}
+EXPORT_SYMBOL(__kfifo_put_user);
+
+/**
* __kfifo_get - gets some data from the FIFO, no locking version
* @fifo: the fifo to be used.
* @buffer: where the data must be copied.
@@ -193,4 +247,58 @@ unsigned int __kfifo_get(struct kfifo *fifo,
return len;
}
+
EXPORT_SYMBOL(__kfifo_get);
+
+/**
+ * __kfifo_get_user - gets some data from the FIFO to userspace
+ * no blocking version
+ * @fifo: the fifo to be used.
+ * @buffer: user space buffer where the data must be copied.
+ * @len: the size of the destination buffer.
+ *
+ * This function copies at most @len bytes from the FIFO into the
+ * @buffer and returns the number of copied bytes.
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these functions.
+ */
+
+int __kfifo_get_user(struct kfifo *fifo,
+ unsigned char __user *buffer, unsigned int len)
+{
+ unsigned int l;
+
+ len = min(len, fifo->in - fifo->out);
+
+ /*
+ * Ensure that we sample the fifo->in index -before- we
+ * start removing bytes from the kfifo.
+ */
+
+ smp_rmb();
+
+ /* first get the data from fifo->out until the end of the buffer */
+ l = min(len, fifo->size - (fifo->out & (fifo->size - 1)));
+
+ if (copy_to_user(buffer, fifo->buffer + (fifo->out & (fifo->size - 1)), l))
+ return -EFAULT;
+
+ /* then get the rest (if any) from the beginning of the buffer */
+ if (copy_to_user(buffer + l, fifo->buffer, len - l))
+ return -EFAULT;
+
+ /*
+ * Ensure that we remove the bytes from the kfifo -before-
+ * we update the fifo->out index.
+ */
+
+ smp_mb();
+
+ fifo->out += len;
+
+ return (int)len;
+}
+
+EXPORT_SYMBOL(__kfifo_get_user);
+
--
http://arhuaco.org
http://emQbit.com
Le lundi 11 juin 2007 ? 16:26 -0500, Nelson Castillo a ?crit :
> Hi.
>
> I just added support for user space buffers in kfifo. I found useful
> __kfifo_get_user to copy data to a user buffer in a read call. I didn't
> like the idea of having an extra buffer.
>
> * Is it ok to add this support?
I suppose it is, however:
> + spin_lock_irqsave(fifo->lock, flags);
> +
> + ret = __kfifo_put_user(fifo, buffer, len);
> +
> + spin_unlock_irqrestore(fifo->lock, flags);
[...]
> +int __kfifo_put_user(struct kfifo *fifo, const unsigned char __user *buffer,
> + unsigned int len)
[...]
> + if(copy_from_user(fifo->buffer + (fifo->in & (fifo->size - 1)),
> + buffer, l))
> + return -EFAULT;
[...]
accessing userspace memory with a spinlock taken (moreover an irqsave()
one) is bad bad bad.
Stelian.
--
Stelian Pop <[email protected]>
On 6/12/07, Stelian Pop <[email protected]> wrote:
> Le lundi 11 juin 2007 ? 16:26 -0500, Nelson Castillo a ?crit :
> > Hi.
> >
> > I just added support for user space buffers in kfifo. I found useful
> > __kfifo_get_user to copy data to a user buffer in a read call. I didn't
> > like the idea of having an extra buffer.
> >
> > * Is it ok to add this support?
>
> I suppose it is, however:
>
> > + spin_lock_irqsave(fifo->lock, flags);
> > +
> > + ret = __kfifo_put_user(fifo, buffer, len);
> > +
> > + spin_unlock_irqrestore(fifo->lock, flags);
> [...]
>
> > +int __kfifo_put_user(struct kfifo *fifo, const unsigned char __user *buffer,
> > + unsigned int len)
> [...]
>
> > + if(copy_from_user(fifo->buffer + (fifo->in & (fifo->size - 1)),
> > + buffer, l))
> > + return -EFAULT;
> [...]
>
> accessing userspace memory with a spinlock taken (moreover an irqsave()
> one) is bad bad bad.
Yes.
Perhaps then only the functions __kfifo_put_user and __kfifo_get_user should
be allowed. Those are the ones I'm using now , anyway.
I added the others for completeness but adding them is probably a mistake.
Regards,
Nelson.-
--
http://arhuaco.org
http://emQbit.com
On 6/12/07, Stelian Pop <[email protected]> wrote:
(cut)
> accessing userspace memory with a spinlock taken (moreover an
> irqsave()
> one) is bad bad bad.
Hi Stelian.
I'm sending the new patch without kfifo_put_user and kfifo_get_user.
Regards,
Nelson.-
Signed-off-by: Nelson Castillo <[email protected]>
---
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 404f446..5c4c416 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -41,8 +41,13 @@ extern struct kfifo *kfifo_alloc(unsigned int size, gfp_t gfp_mask,
extern void kfifo_free(struct kfifo *fifo);
extern unsigned int __kfifo_put(struct kfifo *fifo,
unsigned char *buffer, unsigned int len);
+extern int __kfifo_put_user(struct kfifo *fifo,
+ const unsigned char __user *buffer,
+ unsigned int len);
extern unsigned int __kfifo_get(struct kfifo *fifo,
unsigned char *buffer, unsigned int len);
+extern int __kfifo_get_user(struct kfifo *fifo, unsigned char __user *buffer,
+ unsigned int len);
/**
* __kfifo_reset - removes the entire FIFO contents, no locking version
diff --git a/kernel/kfifo.c b/kernel/kfifo.c
index cee4191..41438ec 100644
--- a/kernel/kfifo.c
+++ b/kernel/kfifo.c
@@ -23,6 +23,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/err.h>
+#include <asm/uaccess.h>
#include <linux/kfifo.h>
/**
@@ -150,6 +151,61 @@ unsigned int __kfifo_put(struct kfifo *fifo,
EXPORT_SYMBOL(__kfifo_put);
/**
+ * __kfifo_put_user - puts some data into the FIFO, from userspace
+ * We don't have a locking version because copy_from_user
+ * might sleep and you must not sleep holding a spinlock.
+ *
+ * @fifo: the fifo to be used.
+ * @buffer: the user space data to be added.
+ * @len: the length of the data to be added.
+ *
+ * This function copies at most @len bytes from the @buffer into
+ * the FIFO depending on the free space, and returns the number of
+ * bytes copied.
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these functions.
+ */
+int __kfifo_put_user(struct kfifo *fifo, const unsigned char __user *buffer,
+ unsigned int len)
+{
+ unsigned int l;
+
+ len = min(len, fifo->size - fifo->in + fifo->out);
+
+ /*
+ * Ensure that we sample the fifo->out index -before- we
+ * start putting bytes into the kfifo.
+ */
+
+ smp_mb();
+
+ /* first put the data starting from fifo->in to buffer end */
+ l = min(len, fifo->size - (fifo->in & (fifo->size - 1)));
+
+ if(copy_from_user(fifo->buffer + (fifo->in & (fifo->size - 1)),
+ buffer, l))
+ return -EFAULT;
+
+
+ /* then put the rest (if any) at the beginning of the buffer */
+ if (copy_from_user(fifo->buffer, buffer + l, len - l))
+ return -EFAULT;
+
+ /*
+ * Ensure that we add the bytes to the kfifo -before-
+ * we update the fifo->in index.
+ */
+
+ smp_wmb();
+
+ fifo->in += len;
+
+ return (int)len;
+}
+EXPORT_SYMBOL(__kfifo_put_user);
+
+/**
* __kfifo_get - gets some data from the FIFO, no locking version
* @fifo: the fifo to be used.
* @buffer: where the data must be copied.
@@ -194,3 +250,56 @@ unsigned int __kfifo_get(struct kfifo *fifo,
return len;
}
EXPORT_SYMBOL(__kfifo_get);
+
+/**
+ * __kfifo_get_user - gets some data from the FIFO to userspace
+ * We don't have a locking version because copy_to_user
+ * might sleep and you must not sleep holding a spinlock.
+ * @fifo: the fifo to be used.
+ * @buffer: user space buffer where the data must be copied.
+ * @len: the size of the destination buffer.
+ *
+ * This function copies at most @len bytes from the FIFO into the
+ * @buffer and returns the number of copied bytes.
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these functions.
+ */
+
+int __kfifo_get_user(struct kfifo *fifo,
+ unsigned char __user *buffer, unsigned int len)
+{
+ unsigned int l;
+
+ len = min(len, fifo->in - fifo->out);
+
+ /*
+ * Ensure that we sample the fifo->in index -before- we
+ * start removing bytes from the kfifo.
+ */
+
+ smp_rmb();
+
+ /* first get the data from fifo->out until the end of the buffer */
+ l = min(len, fifo->size - (fifo->out & (fifo->size - 1)));
+
+ if (copy_to_user(buffer, fifo->buffer + (fifo->out & (fifo->size - 1)), l))
+ return -EFAULT;
+
+ /* then get the rest (if any) from the beginning of the buffer */
+ if (copy_to_user(buffer + l, fifo->buffer, len - l))
+ return -EFAULT;
+
+ /*
+ * Ensure that we remove the bytes from the kfifo -before-
+ * we update the fifo->out index.
+ */
+
+ smp_mb();
+
+ fifo->out += len;
+
+ return (int)len;
+}
+EXPORT_SYMBOL(__kfifo_get_user);
+
--
http://arhuaco.org
http://emQbit.com
Le mardi 12 juin 2007 ? 11:24 -0500, Nelson Castillo a ?crit :
> On 6/12/07, Stelian Pop <[email protected]> wrote:
> (cut)
> > accessing userspace memory with a spinlock taken (moreover an
> > irqsave()
> > one) is bad bad bad.
>
> Hi Stelian.
>
> I'm sending the new patch without kfifo_put_user and kfifo_get_user.
Ok, I have a few formatting/documentation comments:
> + * This function copies at most @len bytes from the @buffer into
> + * the FIFO depending on the free space, and returns the number of
> + * bytes copied.
... or -EFAULT if copy_from_user fails
> + *
> + * Note that with only one concurrent reader and one concurrent
> + * writer, you don't need extra locking to use these functions.
> + */
> +int __kfifo_put_user(struct kfifo *fifo, const unsigned char __user *buffer,
> + unsigned int len)
spacing ?
> + if(copy_from_user(fifo->buffer + (fifo->in & (fifo->size - 1)),
> + buffer, l))
leave a space after the if please
> + * This function copies at most @len bytes from the FIFO into the
> + * @buffer and returns the number of copied bytes.
or -EFAULT
> + *
> + * Note that with only one concurrent reader and one concurrent
> + * writer, you don't need extra locking to use these functions.
> + */
> +
> +int __kfifo_get_user(struct kfifo *fifo,
> + unsigned char __user *buffer, unsigned int len)
spacing.
Other than that I'm ok with this patch.
Thanks,
Stelian.
--
Stelian Pop <[email protected]>
On 6/12/07, Stelian Pop <[email protected]> wrote:
> Le mardi 12 juin 2007 ? 11:24 -0500, Nelson Castillo a ?crit :
> > On 6/12/07, Stelian Pop <[email protected]> wrote:
> > (cut)
> > > accessing userspace memory with a spinlock taken (moreover an
> > > irqsave()
> > > one) is bad bad bad.
> >
> > Hi Stelian.
> >
> > I'm sending the new patch without kfifo_put_user and kfifo_get_user.
>
> Ok, I have a few formatting/documentation comments:
Applied, thanks.
> Other than that I'm ok with this patch.
Submitting.
Regards, Nelson.-
Signed-off-by: Nelson Castillo <[email protected]>
---
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 404f446..5c4c416 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -41,8 +41,13 @@ extern struct kfifo *kfifo_alloc(unsigned int size, gfp_t gfp_mask,
extern void kfifo_free(struct kfifo *fifo);
extern unsigned int __kfifo_put(struct kfifo *fifo,
unsigned char *buffer, unsigned int len);
+extern int __kfifo_put_user(struct kfifo *fifo,
+ const unsigned char __user *buffer,
+ unsigned int len);
extern unsigned int __kfifo_get(struct kfifo *fifo,
unsigned char *buffer, unsigned int len);
+extern int __kfifo_get_user(struct kfifo *fifo, unsigned char __user *buffer,
+ unsigned int len);
/**
* __kfifo_reset - removes the entire FIFO contents, no locking version
diff --git a/kernel/kfifo.c b/kernel/kfifo.c
index cee4191..b11024e 100644
--- a/kernel/kfifo.c
+++ b/kernel/kfifo.c
@@ -23,6 +23,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/err.h>
+#include <asm/uaccess.h>
#include <linux/kfifo.h>
/**
@@ -150,6 +151,61 @@ unsigned int __kfifo_put(struct kfifo *fifo,
EXPORT_SYMBOL(__kfifo_put);
/**
+ * __kfifo_put_user - puts some data into the FIFO, from userspace
+ * We don't have a locking version because copy_from_user
+ * might sleep and you must not sleep holding a spinlock.
+ *
+ * @fifo: the fifo to be used.
+ * @buffer: the user space data to be added.
+ * @len: the length of the data to be added.
+ *
+ * This function copies at most @len bytes from the @buffer into
+ * the FIFO depending on the free space, and returns the number of
+ * bytes copied or -EFAULT if copy_from_user fails.
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these functions.
+ */
+int __kfifo_put_user(struct kfifo *fifo, const unsigned char __user *buffer,
+ unsigned int len)
+{
+ unsigned int l;
+
+ len = min(len, fifo->size - fifo->in + fifo->out);
+
+ /*
+ * Ensure that we sample the fifo->out index -before- we
+ * start putting bytes into the kfifo.
+ */
+
+ smp_mb();
+
+ /* first put the data starting from fifo->in to buffer end */
+ l = min(len, fifo->size - (fifo->in & (fifo->size - 1)));
+
+ if (copy_from_user(fifo->buffer + (fifo->in & (fifo->size - 1)),
+ buffer, l))
+ return -EFAULT;
+
+
+ /* then put the rest (if any) at the beginning of the buffer */
+ if (copy_from_user(fifo->buffer, buffer + l, len - l))
+ return -EFAULT;
+
+ /*
+ * Ensure that we add the bytes to the kfifo -before-
+ * we update the fifo->in index.
+ */
+
+ smp_wmb();
+
+ fifo->in += len;
+
+ return (int)len;
+}
+EXPORT_SYMBOL(__kfifo_put_user);
+
+/**
* __kfifo_get - gets some data from the FIFO, no locking version
* @fifo: the fifo to be used.
* @buffer: where the data must be copied.
@@ -194,3 +250,57 @@ unsigned int __kfifo_get(struct kfifo *fifo,
return len;
}
EXPORT_SYMBOL(__kfifo_get);
+
+/**
+ * __kfifo_get_user - gets some data from the FIFO to userspace
+ * We don't have a locking version because copy_to_user
+ * might sleep and you must not sleep holding a spinlock.
+ * @fifo: the fifo to be used.
+ * @buffer: user space buffer where the data must be copied.
+ * @len: the size of the destination buffer.
+ *
+ * This function copies at most @len bytes from the FIFO into the
+ * @buffer and returns the number of bytes copied or -EFAULT if
+ * copy_to_user fails.
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these functions.
+ */
+
+int __kfifo_get_user(struct kfifo *fifo,
+ unsigned char __user *buffer, unsigned int len)
+{
+ unsigned int l;
+
+ len = min(len, fifo->in - fifo->out);
+
+ /*
+ * Ensure that we sample the fifo->in index -before- we
+ * start removing bytes from the kfifo.
+ */
+
+ smp_rmb();
+
+ /* first get the data from fifo->out until the end of the buffer */
+ l = min(len, fifo->size - (fifo->out & (fifo->size - 1)));
+
+ if (copy_to_user(buffer, fifo->buffer + (fifo->out & (fifo->size - 1)), l))
+ return -EFAULT;
+
+ /* then get the rest (if any) from the beginning of the buffer */
+ if (copy_to_user(buffer + l, fifo->buffer, len - l))
+ return -EFAULT;
+
+ /*
+ * Ensure that we remove the bytes from the kfifo -before-
+ * we update the fifo->out index.
+ */
+
+ smp_mb();
+
+ fifo->out += len;
+
+ return (int)len;
+}
+EXPORT_SYMBOL(__kfifo_get_user);
+
--
http://arhuaco.org
http://emQbit.com
Le mardi 12 juin 2007 ? 21:21 -0500, Nelson Castillo a ?crit :
> On 6/12/07, Stelian Pop <[email protected]> wrote:
> > Le mardi 12 juin 2007 ? 11:24 -0500, Nelson Castillo a ?crit :
> > > On 6/12/07, Stelian Pop <[email protected]> wrote:
> > > (cut)
> > > > accessing userspace memory with a spinlock taken (moreover an
> > > > irqsave()
> > > > one) is bad bad bad.
> > >
> > > Hi Stelian.
> > >
> > > I'm sending the new patch without kfifo_put_user and kfifo_get_user.
> >
> > Ok, I have a few formatting/documentation comments:
>
> Applied, thanks.
>
> > Other than that I'm ok with this patch.
>
> Submitting.
>
> Regards, Nelson.-
>
> Signed-off-by: Nelson Castillo <[email protected]>
Signed-off-by: Stelian Pop <[email protected]>
Thanks,
Stelian.
--
Stelian Pop <[email protected]>