Re: [driver-core:debugfs_cleanup 4/5] fs/d_path.c:59 prepend() warn: unsigned 'p->len' is never less than zero.
On Sat, Jan 01, 2022 at 02:08:54PM +0100, Greg Kroah-Hartman wrote: > On Fri, Dec 31, 2021 at 07:35:07PM +0000, Al Viro wrote: > > On Sat, Jan 01, 2022 at 01:08:41AM +0800, kernel test robot wrote: > > > tree: > > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git > > > debugfs_cleanup > > > head: a04bbe0a2c7e98669e11a47f94e53dd8228bbeba > > > commit: e95d5bed5d58c2f5352969369827e7135fa2261e [4/5] fs: make > > > d_path-like functions all have unsigned size > > > config: i386-randconfig-m031-20211228 > > > (https://download.01.org/0day-ci/archive/20220101/202201010156.bjvo7gaw-...@intel.com/config) > > > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > > > > > > If you fix the issue, kindly add following tag as appropriate > > > Reported-by: kernel test robot > > > > > > smatch warnings: > > > fs/d_path.c:59 prepend() warn: unsigned 'p->len' is never less than zero. > > > > What do you mean, "unsigned p->len"? > > > > ->len really *can* be negative - that's how running out of buffer is > > indicated. > > > > Greg, I stand by the comment I made back in July - this kind of "hardening" > > is > > useless; there's no legitimate reason to pass a huge buffer length, > > especially > > since there's a limit on the length of pathname any syscall would accept. > > See https://www.spinics.net/lists/linux-fsdevel/msg200370.html for the > > variant I would prefer. > > Sorry, yes, I agree with you, but never deleted this commit from this > "scratch" branch of mine. I'll go rebase the branch and purge it from > the system so that 0-day doesn't hit it anymore, sorry for the noise. OK... I'll probably throw something along the lines of "negative => EINVAL, positive greater than 32Kb - adjust the buffer and length to the last 32Kb of what had been passed" into the misc queue. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [driver-core:debugfs_cleanup 4/5] fs/d_path.c:59 prepend() warn: unsigned 'p->len' is never less than zero.
On Sat, Jan 01, 2022 at 01:08:41AM +0800, kernel test robot wrote: > tree: > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git > debugfs_cleanup > head: a04bbe0a2c7e98669e11a47f94e53dd8228bbeba > commit: e95d5bed5d58c2f5352969369827e7135fa2261e [4/5] fs: make d_path-like > functions all have unsigned size > config: i386-randconfig-m031-20211228 > (https://download.01.org/0day-ci/archive/20220101/202201010156.bjvo7gaw-...@intel.com/config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > smatch warnings: > fs/d_path.c:59 prepend() warn: unsigned 'p->len' is never less than zero. What do you mean, "unsigned p->len"? ->len really *can* be negative - that's how running out of buffer is indicated. Greg, I stand by the comment I made back in July - this kind of "hardening" is useless; there's no legitimate reason to pass a huge buffer length, especially since there's a limit on the length of pathname any syscall would accept. See https://www.spinics.net/lists/linux-fsdevel/msg200370.html for the variant I would prefer. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 05:30:41PM +, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:58:15PM +0200, Christian Brauner wrote:
>
> > They could probably refactor this but I'm not sure why they'd bother. If
> > they fail processing any of those files they end up aborting the
> > whole transaction.
> > (And the original code didn't check the error code btw.)
>
> Wait a sec... What does aborting the transaction do to descriptor table?
>
> Oh, lovely...
>
> binder_apply_fd_fixups() is deeply misguided. What it should do is
> * go through t->fd_fixups, reserving descriptor numbers and
> putting them into t->buffer (and I'd probably duplicate them into
> struct binder_txn_fd_fixup). Cleanup in case of failure: go through
> the list, releasing the descriptors we'd already reserved, doing
> fput() on fixup->file in all entries and freeing the entries as
> we go.
> * On success, go through the list, doing fd_install() and
> freeing the entries.
Something like this:
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..b0c5f7e625f3 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2195,6 +2195,7 @@ static int binder_translate_fd(u32 fd, binder_size_t
fd_offset,
fixup->offset = fd_offset;
trace_binder_transaction_fd_send(t, fd, fixup->offset);
list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
+ fixup->target_fd = -1;
return ret;
@@ -3707,25 +3708,10 @@ static int binder_wait_for_work(struct binder_thread
*thread,
return ret;
}
-/**
- * binder_apply_fd_fixups() - finish fd translation
- * @proc: binder_proc associated @t->buffer
- * @t: binder transaction with list of fd fixups
- *
- * Now that we are in the context of the transaction target
- * process, we can allocate and install fds. Process the
- * list of fds to translate and fixup the buffer with the
- * new fds.
- *
- * If we fail to allocate an fd, then free the resources by
- * fput'ing files that have not been processed and ksys_close'ing
- * any fds that have already been allocated.
- */
-static int binder_apply_fd_fixups(struct binder_proc *proc,
+static int binder_reserve_fds(struct binder_proc *proc,
struct binder_transaction *t)
{
- struct binder_txn_fd_fixup *fixup, *tmp;
- int ret = 0;
+ struct binder_txn_fd_fixup *fixup;
list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
int fd = get_unused_fd_flags(O_CLOEXEC);
@@ -3734,42 +3720,55 @@ static int binder_apply_fd_fixups(struct binder_proc
*proc,
binder_debug(BINDER_DEBUG_TRANSACTION,
"failed fd fixup txn %d fd %d\n",
t->debug_id, fd);
- ret = -ENOMEM;
- break;
+ return -ENOMEM;
}
binder_debug(BINDER_DEBUG_TRANSACTION,
"fd fixup txn %d fd %d\n",
t->debug_id, fd);
trace_binder_transaction_fd_recv(t, fd, fixup->offset);
- fd_install(fd, fixup->file);
- fixup->file = NULL;
+ fixup->target_fd = fd;
if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
fixup->offset, &fd,
- sizeof(u32))) {
- ret = -EINVAL;
- break;
- }
+ sizeof(u32)))
+ return -EINVAL;
}
- list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
- if (fixup->file) {
- fput(fixup->file);
- } else if (ret) {
- u32 fd;
- int err;
-
- err = binder_alloc_copy_from_buffer(&proc->alloc, &fd,
- t->buffer,
- fixup->offset,
- sizeof(fd));
- WARN_ON(err);
- if (!err)
- binder_deferred_fd_close(fd);
+ return 0;
+}
+
+/**
+ * binder_apply_fd_fixups() - finish fd translation
+ * @proc: binder_proc associated @t->buffer
+ * @t: binder transaction with list of fd fixups
+ *
+ * Now that we are in the context of the transaction target
+ * process, we can allocate fds. Process the list of fds to
+ * translate and fixup the buffer with the new fds.
+ *
+ * If we fail to al
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 05:58:15PM +0200, Christian Brauner wrote: > They could probably refactor this but I'm not sure why they'd bother. If > they fail processing any of those files they end up aborting the > whole transaction. > (And the original code didn't check the error code btw.) Wait a sec... What does aborting the transaction do to descriptor table? Oh, lovely... binder_apply_fd_fixups() is deeply misguided. What it should do is * go through t->fd_fixups, reserving descriptor numbers and putting them into t->buffer (and I'd probably duplicate them into struct binder_txn_fd_fixup). Cleanup in case of failure: go through the list, releasing the descriptors we'd already reserved, doing fput() on fixup->file in all entries and freeing the entries as we go. * On success, go through the list, doing fd_install() and freeing the entries. That's it. No rereading from the buffer, no binder_deferred_fd_close() crap, etc. Again, YOU CAN NOT UNDO fd_install(). Ever. Kernel can not decide it shouldn't have put something in descriptor table and take it back. You can't unring that bell. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 06:00:38PM +0200, Christian Brauner wrote:
> (dma_buf_fd() seems like another good candidate. But again, I don't have
> any plans to shove this down anyone's throat.)
Sure, there are candidates for such a helper. Just as there are legitimate
users of anon_inode_getfd().
However, it is *NOT* something we can use as a vehicle for some hooks (pardon
the obscenity); it won't be consistently used in all cases - it simply is not
feasible for many of the fd_install() users.
Incidentally, looking at the user of receive_fd_user(), I would say that it
would be easier to follow in this form:
case VDUSE_IOTLB_GET_ENTRY: {
struct vduse_iotlb_entry entry;
struct vhost_iotlb_map *map;
struct vduse_iova_domain *domain = dev->domain;
struct file *f = NULL;
if (copy_from_user(&entry, argp, sizeof(entry)))
return -EFAULT;
entry.fd = get_unused_fd_flags(perm_to_file_flags(entry.perm));
if (entry.fd < 0)
return entry.fd;
spin_lock(&domain->iotlb_lock);
map = vhost_iotlb_itree_first(domain->iotlb,
entry.start, entry.start + 1);
if (map) {
struct vdpa_map_file *map_file = map->opaque;
f = get_file(map_file->file);
entry.offset = map_file->offset;
entry.start = map->start;
entry.last = map->last;
entry.perm = map->perm;
}
spin_unlock(&domain->iotlb_lock);
if (!f) {
put_unused_fd(entry.fd);
return -EINVAL;
}
if (copy_to_user(argp, &entry, sizeof(entry))) {
put_unused_fd(entry.fd);
fput(f);
return -EFAULT;
}
// point of no return
fd_install(entry.fd, f);
return 0;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
> My point here was more that the _file_ has already been opened _before_
> that call to io_uring_add_task_file(). But any potential non-trivial
> side-effects of opening that file that you correctly pointed out in an
> earlier mail has already happened by that time.
The file comes from io_uring_get_file(), the entire thing is within the
io_ring_ctx constructor and the only side effect there is ->ring_sock
creation. And that stays until the io_ring_ctx is freed. I'm _not_
saying I like io_uring style in general, BTW - in particular,
->ring_sock->file handling is a kludge (as is too much of interation
with AF_UNIX machinery there). But from side effects POV we are fine
there.
> Granted there are more
> obvious examples, e.g. the binder stuff.
>
> int fd = get_unused_fd_flags(O_CLOEXEC);
>
> if (fd < 0) {
> binder_debug(BINDER_DEBUG_TRANSACTION,
>"failed fd fixup txn %d fd %d\n",
>t->debug_id, fd);
> ret = -ENOMEM;
> break;
> }
> binder_debug(BINDER_DEBUG_TRANSACTION,
>"fd fixup txn %d fd %d\n",
>t->debug_id, fd);
> trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> fd_install(fd, fixup->file);
> fixup->file = NULL;
> if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> fixup->offset, &fd,
> sizeof(u32))) {
> ret = -EINVAL;
> break;
> }
... and it's actually broken, since this
/* All copies must be 32-bit aligned and 32-bit size */
if (!check_buffer(alloc, buffer, buffer_offset, bytes))
return -EINVAL;
in binder_alloc_copy_to_buffer() should've been done *before*
fd_install(). If anything, it's an example of the situation when
fd_receive() would be wrong...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 03:42:52PM +0200, Christian Brauner wrote:
> > > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.
> >
> > FWIW, pretty much all ioctls that return descriptor as part of a structure
> > stored to user-supplied address tend to be that way; some don't have any
> > other output fields (in which case they probably would've been better off
> > with just passing the descriptor as return value of ioctl(2)). Those
> > might be served by that receive_fd_user() helper; anything that has several
> > outputs won't be. The same goes for anything that has hard-to-undo
> > operations as part of what they need to do:
> > reserve fd
> > set file up
> > do hard-to-undo stuff
> > install into descriptor table
> > is the only feasible order of operations - reservation can fail, so
> > it must go before the hard-to-undo part and install into descriptor
> > table can't be undone at all, so it must come last. Looks like
> > e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
> > that sort...
>
> If receive_fd() or your receive_fd_user() proposal can replace a chunk
My what proposal? The thing is currently in linux/file.h, put there
by Kees half a year ago...
> of open-coded places in modules where the split between reserving the
> file descriptor and installing it is pointless it's probably already
> worth it.
A helper for use in some of the simplest cases, with big fat warnings
not to touch if the things are not entirely trivial - sure, why not,
same as we have anon_inode_getfd(). But that's a convenience helper,
not a general purpose primitive.
> Random example from io_uring where the file is already opened
> way before (which yes, isn't a module afaik but another place where we
> have that pattern):
>
> static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
> {
> int ret, fd;
>
> fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
> if (fd < 0)
> return fd;
>
> ret = io_uring_add_task_file(ctx);
Huh? It's
ret = io_uring_add_task_file(ctx, file);
in the mainline and I don't see how that sucker could work without having
file passed to it.
> if (ret) {
> put_unused_fd(fd);
> return ret;
> }
> fd_install(fd, file);
> return fd;
> }
... and that's precisely the situation where we have something that is
not obvious how to undo; look into io_uring_add_task_file()...
We have three things to do: (1) reserve a descriptor, (2)
io_uring_add_task_file(),
(3) install the file. (1) and (2) may fail, (1) is trivial to undo, (2) might
be
not, (3) is impossible to undo. So I'd say that in this particular case
io_uring is being perfectly reasonable...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 05:19:50AM +, Al Viro wrote: > On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote: > > and see whether all of them can be switched to simply using > > receive_fd(). I did a completely untested rough sketch to illustrate > > what I meant by using binder and devpts Xie seems to have just picked > > those two. But the change is obviously only worth it if all or nearly > > all callers can be switched over without risk of regression. > > It would most likely simplify quite a few codepaths though especially in > > the error paths since we can get rid of put_unused_fd() cleanup. > > > > But it requires buy in from others obviously. > > Opening a file can have non-trivial side effects; reserving a descriptor > can't. Moreover, if you look at the second hit in your list, you'll see > that we do *NOT* want that combined thing there - fd_install() is > completely irreversible, so we can't do that until we made sure the > reply (struct vtpm_proxy_new_dev) has been successfully copied to > userland. No, your receive_fd_user() does not cover that. > > There's a bunch of other cases like that - the next ones on your list > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc. FWIW, pretty much all ioctls that return descriptor as part of a structure stored to user-supplied address tend to be that way; some don't have any other output fields (in which case they probably would've been better off with just passing the descriptor as return value of ioctl(2)). Those might be served by that receive_fd_user() helper; anything that has several outputs won't be. The same goes for anything that has hard-to-undo operations as part of what they need to do: reserve fd set file up do hard-to-undo stuff install into descriptor table is the only feasible order of operations - reservation can fail, so it must go before the hard-to-undo part and install into descriptor table can't be undone at all, so it must come last. Looks like e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of that sort... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote: > My suggestion was to look at all the places were we currently open-code > this in drivers/: > > drivers/android/binder.c: int fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/char/tpm/tpm_vtpm_proxy.c: fd = get_unused_fd_flags(O_RDWR); > drivers/dma-buf/dma-buf.c: fd = get_unused_fd_flags(flags); > drivers/dma-buf/sw_sync.c: int fd = get_unused_fd_flags(O_CLOEXEC); > drivers/dma-buf/sync_file.c:int fd = get_unused_fd_flags(O_CLOEXEC); > drivers/gpio/gpiolib-cdev.c:fd = get_unused_fd_flags(O_RDONLY | > O_CLOEXEC); > drivers/gpio/gpiolib-cdev.c:fd = get_unused_fd_flags(O_RDONLY | > O_CLOEXEC); > drivers/gpio/gpiolib-cdev.c:fd = get_unused_fd_flags(O_RDONLY | > O_CLOEXEC); > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c: fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/drm_atomic_uapi.c: fence_state->fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/drm_lease.c:fd = get_unused_fd_flags(cl->flags & > (O_CLOEXEC | O_NONBLOCK)); > drivers/gpu/drm/drm_syncobj.c: fd = get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/drm_syncobj.c: int fd = get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c: out_fence_fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: out_fence_fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/msm/msm_gem_submit.c: out_fence_fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/virtio/virtgpu_ioctl.c: out_fence_fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:out_fence_fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/infiniband/core/rdma_core.c:new_fd = > get_unused_fd_flags(O_CLOEXEC); > drivers/media/mc/mc-request.c: fd = get_unused_fd_flags(O_CLOEXEC); > drivers/misc/cxl/api.c: rc = get_unused_fd_flags(flags); > drivers/scsi/cxlflash/ocxl_hw.c:rc = get_unused_fd_flags(flags); > drivers/scsi/cxlflash/ocxl_hw.c:dev_err(dev, "%s: > get_unused_fd_flags failed rc=%d\n", > drivers/tty/pty.c: fd = get_unused_fd_flags(flags); > drivers/vfio/vfio.c:ret = get_unused_fd_flags(O_CLOEXEC); > drivers/virt/nitro_enclaves/ne_misc_dev.c: enclave_fd = > get_unused_fd_flags(O_CLOEXEC); > > and see whether all of them can be switched to simply using > receive_fd(). I did a completely untested rough sketch to illustrate > what I meant by using binder and devpts Xie seems to have just picked > those two. But the change is obviously only worth it if all or nearly > all callers can be switched over without risk of regression. > It would most likely simplify quite a few codepaths though especially in > the error paths since we can get rid of put_unused_fd() cleanup. > > But it requires buy in from others obviously. Opening a file can have non-trivial side effects; reserving a descriptor can't. Moreover, if you look at the second hit in your list, you'll see that we do *NOT* want that combined thing there - fd_install() is completely irreversible, so we can't do that until we made sure the reply (struct vtpm_proxy_new_dev) has been successfully copied to userland. No, your receive_fd_user() does not cover that. There's a bunch of other cases like that - the next ones on your list are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc. So I would consider receive_fd() as an attractive nuisance if it is ever suggested (and available) for wide use... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 08:36:43PM +0100, Matthew Wilcox wrote:
> static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned
> int size)
> {
> char *vto = kmap_atomic(to);
>
> memcpy(vto, vfrom, size);
> kunmap_atomic(vto);
> }
>
> in linux/highmem.h ?
You mean, like
static void memcpy_from_page(char *to, struct page *page, size_t offset, size_t
len)
{
char *from = kmap_atomic(page);
memcpy(to, from + offset, len);
kunmap_atomic(from);
}
static void memcpy_to_page(struct page *page, size_t offset, const char *from,
size_t len)
{
char *to = kmap_atomic(page);
memcpy(to + offset, from, len);
kunmap_atomic(to);
}
static void memzero_page(struct page *page, size_t offset, size_t len)
{
char *addr = kmap_atomic(page);
memset(addr + offset, 0, len);
kunmap_atomic(addr);
}
in lib/iov_iter.c? FWIW, I don't like that "highpage" in the name and
highmem.h as location - these make perfect sense regardless of highmem;
they are normal memory operations with page + offset used instead of
a pointer...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[git pull] uaccess comedi compat
comedi compat ioctls done saner (killing the single biggest pile
of __get_user/__put_user outside of arch/* in process).
The following changes since commit 8f3d9f354286745c751374f5f1fcafee6b3f3136:
Linux 5.7-rc1 (2020-04-12 12:35:55 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git uaccess.comedi
for you to fetch changes up to bac42fb21259783cb748ae54227a5e755340a396:
comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat
(2020-05-29 10:06:01 -0400)
Al Viro (10):
comedi: move compat ioctl handling to native fops
comedi: get rid of indirection via translated_ioctl()
comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO
compat
comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO
compat
comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat
comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST
compat
comedi: lift copy_from_user() into callers of __comedi_get_user_cmd()
comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller
comedi: do_cmd_ioctl(): lift copyin/copyout into the caller
comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST}
compat
drivers/staging/comedi/Makefile | 1 -
drivers/staging/comedi/comedi_compat32.c | 455 -
drivers/staging/comedi/comedi_compat32.h | 28 --
drivers/staging/comedi/comedi_fops.c | 564 +--
drivers/staging/comedi/comedi_internal.h | 2 +-
drivers/staging/comedi/range.c | 17 +-
6 files changed, 467 insertions(+), 600 deletions(-)
delete mode 100644 drivers/staging/comedi/comedi_compat32.c
delete mode 100644 drivers/staging/comedi/comedi_compat32.h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user()
On Thu, Mar 05, 2020 at 10:03:25AM +0100, Rasmus Villemoes wrote: > Does copy_from_user guarantee to zero-initialize the remaining buffer if > copying fails partway through? That's guaranteed, short of raw_copy_from_user() being completely broken. What raw_copy_from_user() implementation must guarantee is that if raw_copy_from_user(to, from, N) returns N - n, then * 0 <= n <= N * all attempted reads had been within the range [from .. from + N - 1] * all stores had been to the range [to .. to + n - 1] and every byte within that range had been overwritten * for all k in [0 .. n - 1], the value stored at to[k] by the end of the call is equal to the value that would've been possible to read from from[k] at some point during the call. In particular, for all bytes in range [from .. from + n - 1] there had been a successful read of some object containing that byte. * if everything in [from .. from + N - 1] is readable, the call will copy the entire range into [to .. to + N - 1] and return 0. Provided that, copy_from_user() will leave no uninitialized data in destination object in any case, success or no success. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wfx: add gcc extension __force cast
On Mon, Nov 11, 2019 at 08:28:52PM +, Al Viro wrote:
> So it smells like a remote buffer overrun, little-endian or not.
> And at that point I would start looking for driver original authors with
> some rather pointed questions about the validation of data and lack
> thereof.
>
> BTW, if incoming packets are fixed-endian, I would expect more bugs on
> big-endian hosts - wfx_tx_confirm_cb() does things like
> tx_info->status.tx_time =
> arg->media_delay - arg->tx_queue_delay;
> with media_delay and tx_queue_delay both being 32bit fields in the
> incoming packet. So another question to the authors (or documentation,
> or direct experiments) is what endianness do various fields in the incoming
> data have. We can try and guess, but...
More fun:
int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id, void *val,
size_t val_len)
{
int ret;
struct hif_msg *hif;
int buf_len = sizeof(struct hif_cnf_read_mib) + val_len;
struct hif_req_read_mib *body = wfx_alloc_hif(sizeof(*body), &hif);
struct hif_cnf_read_mib *reply = kmalloc(buf_len, GFP_KERNEL);
OK, allocated request and reply buffers, by the look of it; request one
being struct hif_msg with struct hif_req_read_mib for payload
and reply - struct hif_cnf_read_mib {
uint32_t status;
uint16_t mib_id;
uint16_t length;
uint8_tmib_data[];
} with val_len bytes in mib_data.
body->mib_id = cpu_to_le16(mib_id);
wfx_fill_header(hif, vif_id, HIF_REQ_ID_READ_MIB, sizeof(*body));
Filled request, {.len = cpu_to_le16(4 + 4),
.id = HIF_REQ_ID_READ_MIB,
.interface = vif_id,
.body = {
.mib_id = cpu_to_le16(mib_id)
}
}
Note that mib_id is host-endian here; what we send is little-endian.
ret = wfx_cmd_send(wdev, hif, reply, buf_len, false);
send it, get reply
if (!ret && mib_id != reply->mib_id) {
Wha...? Now we are comparing two bytes at offset 4 into reply with a
host-endian
value? Oh, well...
dev_warn(wdev->dev, "%s: confirmation mismatch request\n",
__func__);
ret = -EIO;
}
if (ret == -ENOMEM)
dev_err(wdev->dev, "buffer is too small to receive %s (%zu <
%d)\n",
get_mib_name(mib_id), val_len, reply->length);
if (!ret)
memcpy(val, &reply->mib_data, reply->length);
What. The. Hell?
We are copying data from the reply. Into caller-supplied object.
With length taken from the same reply and no validation even
attempted? Not even "um, maybe we shouldn't copy more than the caller
told us to copy, especially since that's as much as there is in the
source of that memcpy"?
And that's besides the endianness questions. Note that getting the
endianness wrong here is just about certain to blow up - small value
will be misinterpreted by factor of 256.
In any case, even if this is talking to firmware on a card, that's
an unhealthy degree of trust, especially since the same function
does consider the possibility of bogus replies.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wfx: add gcc extension __force cast
On Mon, Nov 11, 2019 at 01:51:33PM +, Jules Irenge wrote:
>
> > NAK. force-cast (and it's not a gcc extension, BTW - it's sparse) is
> > basically
> > "I know better; the code is right, so STFU already". *IF*
> > counters.count_...
> > is really little-endian 32bit, then why isn't it declared that way? And if
> > it's host-endian, you've just papered over a real bug here.
> >
> > As a general rule "fix" doesn't mean "tell it to shut up"...
> >
>
> Thanks for the comments, I have updated but I have a mixed mind on the
> __le32. I have to read more about it.
>
> I would appreciate if you can comment again on the update.
>From the look at the driver, it seems that all these counters are a part of
structure that is read from the hardware and only used as little-endian.
So just declare all fields in struct hif_mib_extended_count_table as
__le32; easy enough. Looking a bit further, the same goes for
struct hif_mib_bcn_filter_table ->num_of_info_elmts
struct hif_mib_keep_alive_period ->keep_alive_period (__le16)
struct hif_mib_template_frame ->frame_length (__le16)
struct hif_mib_set_association_mode ->basic_rate_set (__le32)
struct hif_req_update_ie ->num_i_es (__le16)
struct hif_req_write_mib ->mib_id, ->length (__le16 both)
struct hif_req_read_mib ->mib_id (__le16)
struct hif_req_configuration ->length (__le16)
With those annotated we are left with
drivers/staging/wfx/bh.c:73:35: warning: incorrect type in argument 1
(different base types)
drivers/staging/wfx/bh.c:73:35:expected restricted __le16 const [usertype]
*p
drivers/staging/wfx/bh.c:73:35:got unsigned short [usertype] *
drivers/staging/wfx/bh.c:106:41: warning: cast to restricted __le32
drivers/staging/wfx/bh.c:175:22: warning: cast to restricted __le16
drivers/staging/wfx/hwio.c:199:16: warning: cast from restricted __le32
drivers/staging/wfx/hwio.c:199:14: warning: incorrect type in assignment
(different base types)
drivers/staging/wfx/hwio.c:199:14:expected unsigned int [usertype]
drivers/staging/wfx/hwio.c:199:14:got restricted __le32 [usertype]
drivers/staging/wfx/hif_tx.c:36:18: warning: incorrect type in assignment
(different base types)
drivers/staging/wfx/hif_tx.c:36:18:expected unsigned short [usertype] len
drivers/staging/wfx/hif_tx.c:36:18:got restricted __le16 [usertype]
drivers/staging/wfx/data_tx.c:646:22: warning: incorrect type in assignment
(different base types)
drivers/staging/wfx/data_tx.c:646:22:expected unsigned short [usertype] len
drivers/staging/wfx/data_tx.c:646:22:got restricted __le16 [usertype]
drivers/staging/wfx/hif_tx_mib.h:111:27: warning: incorrect type in initializer
(different base types)
drivers/staging/wfx/hif_tx_mib.h:111:27:expected unsigned int [usertype]
enable
drivers/staging/wfx/hif_tx_mib.h:111:27:got restricted __le32 [usertype]
drivers/staging/wfx/hif_tx_mib.h:112:30: warning: incorrect type in initializer
(different base types)
drivers/staging/wfx/hif_tx_mib.h:112:30:expected unsigned int [usertype]
bcn_count
drivers/staging/wfx/hif_tx_mib.h:112:30:got restricted __le32 [usertype]
drivers/staging/wfx/hif_tx_mib.h:34:38: warning: incorrect type in initializer
(different base types)
drivers/staging/wfx/hif_tx_mib.h:34:38:expected unsigned char [usertype]
wakeup_period_max
drivers/staging/wfx/hif_tx_mib.h:34:38:got restricted __le16 [usertype]
drivers/staging/wfx/main.c:108:39: warning: incorrect type in initializer
(different base types)
drivers/staging/wfx/main.c:108:39:expected restricted __le16 [usertype]
rx_highest
drivers/staging/wfx/main.c:108:39:got int
drivers/staging/wfx/./traces.h:155:1: warning: incorrect type in argument 1
(different base types)
drivers/staging/wfx/./traces.h:155:1:expected restricted __le16 const
[usertype] *p
drivers/staging/wfx/./traces.h:155:1:got unsigned short [usertype] *
drivers/staging/wfx/./traces.h:155:1: warning: incorrect type in argument 1
(different base types)
drivers/staging/wfx/./traces.h:155:1:expected restricted __le16 const
[usertype] *p
drivers/staging/wfx/./traces.h:155:1:got unsigned short [usertype] *
and that's where the real bugs start to show up; leaving the misbegotten
forest of macros in misbegotten tracing shite aside, we have this:
static const struct ieee80211_supported_band wfx_band_2ghz = {
.channels = wfx_2ghz_chantable,
.n_channels = ARRAY_SIZE(wfx_2ghz_chantable),
.bitrates = wfx_rates,
.n_bitrates = ARRAY_SIZE(wfx_rates),
.ht_cap = {
// Receive caps
.cap = IEEE80211_HT_CAP_GRN_FLD | IEEE80211_HT_CAP_SGI_20 |
IEEE80211_HT_CAP_MAX_AMSDU | (1 <<
IEEE80211_HT_CAP_RX_STBC_SHIFT),
.ht_supported = 1,
.ampdu_factor = IEEE80211_HT_MAX_AMPDU_16K,
.ampdu_density = IEEE80211_HT_MPDU_DENSITY_NONE,
.mcs = {
.rx_mask = { 0xFF }, // MCS0 to MCS7
Re: [PATCH] staging: wfx: add gcc extension __force cast
On Fri, Nov 08, 2019 at 11:38:37PM +, Jules Irenge wrote: > Add gcc extension __force and __le32 cast to fix warning issued by Sparse > tool."warning: cast to restricted __le32" > > Signed-off-by: Jules Irenge > --- > drivers/staging/wfx/debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/wfx/debug.c b/drivers/staging/wfx/debug.c > index 0a9ca109039c..aa7b2dd691b9 100644 > --- a/drivers/staging/wfx/debug.c > +++ b/drivers/staging/wfx/debug.c > @@ -72,7 +72,7 @@ static int wfx_counters_show(struct seq_file *seq, void *v) > return -EIO; > > #define PUT_COUNTER(name) \ > - seq_printf(seq, "%24s %d\n", #name ":", > le32_to_cpu(counters.count_##name)) > + seq_printf(seq, "%24s %d\n", #name ":", le32_to_cpu((__force > __le32)(counters.count_##name))) NAK. force-cast (and it's not a gcc extension, BTW - it's sparse) is basically "I know better; the code is right, so STFU already". *IF* counters.count_... is really little-endian 32bit, then why isn't it declared that way? And if it's host-endian, you've just papered over a real bug here. As a general rule "fix" doesn't mean "tell it to shut up"... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: Capitalize macro name to fix camelcase checkpatch warning
On Sun, Oct 06, 2019 at 07:49:03PM +0100, Jules Irenge wrote: [mA vs. MA] Table 5. SI prefixes Factor NameSymbol 10^6megaM 10^-3 milli m Confusing one for another (especially for electrical units) can be... spectacular. FYI, 1mA is more or less what you get if you lick the terminals of a 9V battery; 1MA is about 30 times the current in typical lightning strike... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: cleanup braces for if/else statements
On Tue, Sep 03, 2019 at 06:47:32PM +0200, Valentin Vidic wrote:
> + } else if (uni == 0x) {
> skip = TRUE;
While we are at it, could you get rid of that 'TRUE' macro?
Or added
#define THE_TRUTH_AND_THATS_CUTTIN_ME_OWN_THROAT true
if you want to properly emphasize it...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/24] erofs: add super block operations
On Sun, Jul 21, 2019 at 11:08:42AM +0800, Gao Xiang wrote: > It is for debugging use as you said below, mainly for our internal > testers whose jobs are > to read kmsg logs and catch kernel problems. sb->s_id (device number) > maybe not > straight-forward for them compared with dev_name... Huh? ->s_id is something like "sdb7" - it's bdev_name(), not a device number... > The initial purpose of erofs_mount_private was to passing multi private > data from erofs_mount > to erofs_read_super, which was written before fs_contest was introduced. That has nothing to do with fs_context (well, other than fs_context conversions affecting the code very close to that). > I agree with you, it seems better to just use s_id in community and > delete erofs_mount_private stuffs... > Yet I don't look into how to use new fs_context, could I keep using > legacy mount interface and fix them all? Sure. > I guess if I don't misunderstand, that is another suggestion -- in > short, leave all destructors to .kill_sb() and > cleanup fill_super(). Just be careful with that iput() there - AFAICS, if fs went live (i.e. if ->s_root is non-NULL), you really need it done only from put_super(); OTOH, for the case of NULL ->s_root ->put_super() won't be called at all, so in that case you need it directly in ->kill_sb(). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/24] erofs: add super block operations
On Thu, Jul 11, 2019 at 10:57:34PM +0800, Gao Xiang wrote:
> This commit adds erofs super block operations, including (u)mount,
> remount_fs, show_options, statfs, in addition to some private
> icache management functions.
Could you explain what's the point of this
> + /* save the device name to sbi */
> + sbi->dev_name = __getname();
> + if (!sbi->dev_name) {
> + err = -ENOMEM;
> + goto err_devname;
> + }
> +
> + snprintf(sbi->dev_name, PATH_MAX, "%s", dev_name);
> + sbi->dev_name[PATH_MAX - 1] = '\0';
... and this?
> +struct erofs_mount_private {
> + const char *dev_name;
> + char *options;
> +};
> +
> +/* support mount_bdev() with options */
> +static int erofs_fill_super(struct super_block *sb,
> + void *_priv, int silent)
> +{
> + struct erofs_mount_private *priv = _priv;
> +
> + return erofs_read_super(sb, priv->dev_name,
> + priv->options, silent);
> +}
> +
> +static struct dentry *erofs_mount(
> + struct file_system_type *fs_type, int flags,
> + const char *dev_name, void *data)
> +{
> + struct erofs_mount_private priv = {
> + .dev_name = dev_name,
> + .options = data
> + };
> +
> + return mount_bdev(fs_type, flags, dev_name,
> + &priv, erofs_fill_super);
> +}
AFAICS, the only use of sbi->dev_name is debugging printks and
all of those have sb->s_id available, with device name stored
in there. Which makes the whole thing bloody weird - what's
wrong with simply passing data to mount_bdev (instead of
&priv), folding erofs_read_super() into erofs_fill_super(),
replacing sbi->dev_name with sb->s_id and killing sbi->dev_name,
along with the associated allocation, freeing, handling of
allocation failure, etc.?
For drivers/staging location that would be (compile-tested only)
the diff below. I suspect that you could simplify fill_super
a bit further if you added ->kill_sb() along the lines of
sbi = EROFS(sb);
#ifdef EROFS_FS_HAS_MANAGED_CACHE
if (sbi && !sb->s_root)
iput(sbi->managed_cache);
#endif
kill_block_super(sb);
kfree(sbi);
and took freeing sbi out of your ->put_super(). Then fill_super()
would simply return -E... on all failure exits, leaving all cleanup
to ->kill_sb(). E.g. initialization of the same ->managed_cache
would become
#ifdef EROFS_FS_HAS_MANAGED_CACHE
inode = erofs_init_managed_cache(sb);
if (IS_ERR(inode))
return PTR_ERR(inode);
sbi->managed_cache = inode;
#endif
etc. Matter of taste, but IME if destructor parallels the cleanups on failure
exits in constructor it often makes sense to make use of that and kill the
duplication... Anyway, that's a separate store; sbi->dev_name is a lot more
obvious one.
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 382258fc124d..16bab07e69d8 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -117,8 +117,6 @@ struct erofs_sb_info {
u8 volume_name[16]; /* volume name */
u32 requirements;
- char *dev_name;
-
unsigned int mount_opt;
unsigned int shrinker_run_no;
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index cadbcc11702a..a6ee69d0ce45 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -367,15 +367,14 @@ static struct inode *erofs_init_managed_cache(struct
super_block *sb)
#endif
-static int erofs_read_super(struct super_block *sb,
- const char *dev_name,
+static int erofs_fill_super(struct super_block *sb,
void *data, int silent)
{
struct inode *inode;
struct erofs_sb_info *sbi;
int err = -EINVAL;
- infoln("read_super, device -> %s", dev_name);
+ infoln("read_super, device -> %s", sb->s_id);
infoln("options -> %s", (char *)data);
if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
@@ -453,20 +452,10 @@ static int erofs_read_super(struct super_block *sb,
goto err_iget;
}
- /* save the device name to sbi */
- sbi->dev_name = __getname();
- if (!sbi->dev_name) {
- err = -ENOMEM;
- goto err_devname;
- }
-
- snprintf(sbi->dev_name, PATH_MAX, "%s", dev_name);
- sbi->dev_name[PATH_MAX - 1] = '\0';
-
erofs_register_super(sb);
if (!silent)
- infoln("mounted on %s with opts: %s.", dev_name,
+ infoln("mounted on %s with opts: %s.", sb->s_id,
(char *)data);
return 0;
/*
@@ -474,9 +463,6 @@ static int erofs_read_super(struct super_block *sb,
* the following name convention, thus new features
* can be integrated easily without renaming labels.
*/
-err_devname:
- dput(sb->s_root);
- sb->s_root = NULL;
Re: Procedure questions - new filesystem driver..
On Mon, Jul 08, 2019 at 08:37:42PM -0400, Valdis Klētnieks wrote: > I have an out-of-tree driver for the exfat file system that I beaten into > shape > for upstreaming. The driver works, and passes sparse and checkpatch (except > for a number of line-too-long complaints). > > Do you want this taken straight to the fs/ tree, or through drivers/staging? First of all, post it... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation
On Tue, Apr 30, 2019 at 09:32:20AM -0400, Sven Van Asbroeck wrote:
> On Tue, Apr 30, 2019 at 12:19 AM Al Viro wrote:
> >
> > ... not that there's much sense keeping ->fieldbus_type in host-endian,
> > while we are at it.
>
> Interesting! Suppose we make device->fieldbus_type bus-endian.
> Then the endinan-ness conversion either needs to happen in
> bus_match() (and we'd have to convert endianness each time
> this function is called).
> Or, we make driver->fieldbus_type bus-endian also, then there
> is no need for conversion... but the driver writer has to remember
> to specify this in bus endianness:
>
> static struct anybuss_client_driver profinet_driver = {
> .probe = ...,
> .fieldbus_type = endian convert?? (0x0089),
> };
>
> Which pushes bus implementation details onto the
> client driver writer? Also, how to convert a constant
> to a specific endianness in a static initializer?
cpu_to_be16() or htons() - either will be fine there.
On little-endian you'll get
htons(0x0089) =>
___htons(0x0089) =>
__cpu_to_be16(0x0089) =>
((__force __be16)__swab16((0x0089))) =>
((__be16)(__builtin_constant_p((__u16)((0x0089))) ?
___constant_swab16((0x0089)) : __fswab16((0x0089))) =>
((__be16)(__builtin_constant_p((__u16)((0x0089))) ?
((__u16)__u16)((0x0089)) & (__u16)0x00ffU) << 8) |
(((__u16)((0x0089)) & (__u16)0xff00U) >> 8))) :
__fswab16((0x0089)))
and once the preprocessor has produced that, from compiler POV we have
a constant expression as argument of __builtin_constant_p(), so it
evaluates as true, reducing the whole thing to
((__be16)(((__u16)__u16)((0x0089)) & (__u16)0x00ffU) << 8) |
(((__u16)((0x0089)) & (__u16)0xff00U) >> 8))) )
i.e. (__be16)0x8900. On big-endian expansion will be different,
resulting in (__be16)0x0089...
IOW, you can use endianness convertors in static initializers; things
like
struct sockaddr_in addr = {.sin_addr.s_addr = htonl(0x7f01),
.sin_port = htons(25),
.sin_family = AF_INET};
are fine kernel-side (from the compiler POV, that is - something
trying to speak SMTP in the kernel code would obviously be a bad sign).
As for having to remember - sparse will complain about endianness mismatches
in initializer...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation
On Tue, Apr 30, 2019 at 05:33:10AM +0200, Nicholas Mc Guire wrote: > ok - my bad thn - I had assumed that using __force is reasonable > if the handling is correct and its a localized conversoin only > like var = be16_to_cpu(var) which evaded introducing additinal > variables just to have different types but no different function. If compiler can't recognize that in T1 v1; T2 v2; code using v1, but not v2 v2 = f(v1); code using v2, but not v1 it can use the same memory for v1 and v2, file a bug against the compiler. Or stop using that toy altogether - that kind of optimizations is early 60s stuff and any real compiler will handle that. Both gcc and clang certainly do handle that. Another thing they handle is figuring out that be16_to_cpu() et.al. are pure functions, so f(be16_to_cpu(n)); no modifications of n g(be16_to_cpu(n)); doesn't need to have le16_to_cpu recalculated. IOW, that particular code could as well have been dev_info(dev, "Fieldbus type: %04X", be16_to_cpu(fieldbus_type)); ... cd->client->fieldbus_type = be16_to_cpu(fieldbus_type); ... not that there's much sense keeping ->fieldbus_type in host-endian, while we are at it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation
On Tue, Apr 30, 2019 at 04:22:38AM +0200, Nicholas Mc Guire wrote: > On Mon, Apr 29, 2019 at 10:03:36AM -0400, Sven Van Asbroeck wrote: > > On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire wrote: > > > > > > V2: As requested by Sven Van Asbroeck make the > > > impact of the patch clear in the commit message. > > > > Thank you, but did you miss my comment about creating a local variable > > instead? See: > > https://lkml.org/lkml/2019/4/28/97 > > Did not miss it - I just don't think that makes it any more > understandable - the __force __be16 makes it clear I believe > that this is correct, sparse does not like this though - so tell > sparse. ... to STFU, 'cause you know better. The trouble is, how do we (or yourself a year or two later) know *why* it is correct? Worse, how do we (or yourself, etc.) know if a change about to be done to the code won't invalidate the proof of yours? > The local variable would need to be explained as it is > functionally not necessary - therefor I find it more confusing > that using __force here. What's confusing is mixing host- and fixed-endian values in the same variable at different times. Treat those as unrelated types that happen to have the same sizeof. Quite a few of __force instances in the tree should be taken out and shot. Don't add to their number. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 12/26] compat_ioctl: move more drivers to compat_ptr_ioctl
On Thu, Apr 25, 2019 at 05:55:23PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 25, 2019 at 5:35 PM Al Viro wrote:
> >
> > On Thu, Apr 25, 2019 at 12:21:53PM -0300, Mauro Carvalho Chehab wrote:
> >
> > > If I understand your patch description well, using compat_ptr_ioctl
> > > only works if the driver is not for s390, right?
> >
> > No; s390 is where "oh, just set ->compat_ioctl same as ->unlocked_ioctl
> > and be done with that; compat_ptr() is a no-op anyway" breaks. IOW,
> > s390 is the reason for having compat_ptr_ioctl() in the first place;
> > that thing works on all biarch architectures, as long as all stuff
> > handled by ->ioctl() takes pointer to arch-independent object as
> > argument. IOW,
> > argument ignored => OK
> > any arithmetical type => no go, compat_ptr() would bugger it
> > pointer to int => OK
> > pointer to string => OK
> > pointer to u64 => OK
> > pointer to struct {u64 addr; char s[11];} => OK
>
> To be extra pedantic, the 'struct {u64 addr; char s[11];} '
> case is also broken on x86, because sizeof (obj) is smaller
> on i386, even though the location of the members are
> the same. i.e. you can copy_from_user() this, but not
> copy_to_user(), which overwrites 4 bytes after the end of
> the 20-byte user structure.
D'oh! FWIW, it might be worth putting into Documentation/ somewhere;
basically, what is and what isn't biarch-neutral.
Or arch-neutral, for that matter - it's very close. The only real
exception, IIRC, is an extra twist on m68k, where int behaves
like x86 long long - its alignment is only half its size, so
sizeof(struct {char c; int x;}) is 6, not 8 as everywhere
else. Irrelevant for biarch, thankfully (until somebody gets insane
enough to implement 64bit coldfire, kernel port for it *and* biarch
support for m68k binaries on that thing, that is)...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 12/26] compat_ioctl: move more drivers to compat_ptr_ioctl
On Thu, Apr 25, 2019 at 12:21:53PM -0300, Mauro Carvalho Chehab wrote:
> If I understand your patch description well, using compat_ptr_ioctl
> only works if the driver is not for s390, right?
No; s390 is where "oh, just set ->compat_ioctl same as ->unlocked_ioctl
and be done with that; compat_ptr() is a no-op anyway" breaks. IOW,
s390 is the reason for having compat_ptr_ioctl() in the first place;
that thing works on all biarch architectures, as long as all stuff
handled by ->ioctl() takes pointer to arch-independent object as
argument. IOW,
argument ignored => OK
any arithmetical type => no go, compat_ptr() would bugger it
pointer to int => OK
pointer to string => OK
pointer to u64 => OK
pointer to struct {u64 addr; char s[11];} => OK
pointer to long => needs explicit handler
pointer to struct {void *addr; char s[11];} => needs explicit handler
pointer to struct {int x; u64 y;} => needs explicit handler on amd64
For "just use ->unlocked_ioctl for ->ioctl" we have
argument ignored => OK
any arithmetical type => OK
any pointer => instant breakage on s390, in addtion to cases that break
with compat_ptr_ioctl().
Probably some form of that ought to go into commit message for
compat_ptr_ioctl()
introduction...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] tty: Fix WARNING in tty_set_termios
On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
> tty_set_termios() has the following WARMN_ON which can be triggered with a
> syscall to invoke TIOCGETD __NR_ioctl.
>
> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> tty->driver->subtype == PTY_TYPE_MASTER);
> Reference:
> https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
>
> A simple change would have been to print error message instead of WARN_ON.
> However, the callers assume that tty_set_termios() always returns 0 and
> don't check return value. The complete solution is fixing all the callers
> to check error and bail out to fix the WARN_ON.
>
> This fix changes tty_set_termios() to return error and all the callers
> to check error and bail out. The reproducer is used to reproduce the
> problem and verify the fix.
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool
> enable)
> status = tty_set_termios(tty, &ktermios);
> BT_DBG("Disabling hardware flow control: %s",
> status ? "failed" : "success");
> + if (status)
> + return;
Can that ldisc end up set on pty master? And does it make any sense there?
> diff --git a/drivers/tty/serdev/serdev-ttyport.c
> b/drivers/tty/serdev/serdev-ttyport.c
> index fa1672993b4c..29b51370deac 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -136,7 +136,9 @@ static int ttyport_open(struct serdev_controller *ctrl)
> ktermios.c_cflag |= CRTSCTS;
> /* Hangups are not supported so make sure to ignore carrier detect. */
> ktermios.c_cflag |= CLOCAL;
> - tty_set_termios(tty, &ktermios);
> + ret = tty_set_termios(tty, &ktermios);
How can _that_ happen to pty master?
> diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
> index 78fe622eba65..9978c21ce34d 100644
> --- a/net/nfc/nci/uart.c
> +++ b/net/nfc/nci/uart.c
> @@ -447,6 +447,7 @@ void nci_uart_set_config(struct nci_uart *nu, int
> baudrate, int flow_ctrl)
> else
> new_termios.c_cflag &= ~CRTSCTS;
>
> + /* FIXME tty_set_termios() could return error */
Ditto for this one.
IOW, I don't believe that this patch makes any sense. If anything,
we need to prevent unconditional tty_set_termios() on the path
that *does* lead to calling it for pty.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/5] binderfs: debug galore
On Fri, Jan 18, 2019 at 03:53:39PM +0100, Christian Brauner wrote:
> Hey everyone,
>
> Al gave me a really helpful review of binderfs and pointed out a range
> of bugs. The most obvious and serious ones have fortunately already been
> taken care of by patches sitting in Greg's char-misc-linus tree. The
> others are hopefully all covered in this patchset.
BTW, binderfs_binder_device_create() looks rather odd - it would be easier
to do this:
inode_lock(d_inode(root));
/* look it up */
dentry = lookup_one_len(name, root, strlen(name));
if (IS_ERR(dentry)) {
/* some kind of error (ENOMEM, permissions) - report */
inode_unlock(d_inode(root));
ret = PTR_ERR(dentry);
goto err;
}
if (d_really_is_positive(dentry)) {
/* already exists */
dput(dentry);
inode_unlock(d_inode(root));
ret = -EEXIST;
goto err;
}
inode->i_private = device;
... and from that point on - as in your variant. Another thing in there:
name = kmalloc(name_len, GFP_KERNEL);
if (!name)
goto err;
strscpy(name, req->name, name_len);
is an odd way to go; more straightforward would be
req->name[BINDERFS_MAX_NAME] = '\0';/* NUL-terminate */
name = kmemdup(req->name, sizeof(req->name), GEP_KERNEL);
if (!name)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] binderfs: rework binderfs_fill_super()
On Fri, Jan 18, 2019 at 03:53:42PM +0100, Christian Brauner wrote:
> static int binderfs_fill_super(struct super_block *sb, void *data, int
> silent)
> {
> + int ret;
> struct binderfs_info *info;
> - int ret = -ENOMEM;
> struct inode *inode = NULL;
> struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
>
> @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb,
> void *data, int silent)
> sb->s_op = &binderfs_super_ops;
> sb->s_time_gran = 1;
>
> - info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> - if (!info)
> - goto err_without_dentry;
> + sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> + if (!sb->s_fs_info)
> + return -ENOMEM;
> + info = sb->s_fs_info;
... and that's when you should grab ipcns reference and stick it into
info->ipc_ns, to match the logics in binderfs_kill_super().
Otherwise the failure above
> ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> if (ret)
> - goto err_without_dentry;
> + return ret;
... or here leaves you with an ipcns leak.
Destructor does
if ->s_fs_info is non-NULL
release ->s_fs_info->ipc_ns
free ->s_fs_info
so constructor should not leave object in a state when ipcns is already
grabbed, but not stored in ->s_fs_info->ipc_ns (including the case of
allocation failure leaving it with NULL ->s_fs_info).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/5] binderfs: prevent renaming the control dentry
On Fri, Jan 18, 2019 at 03:53:41PM +0100, Christian Brauner wrote:
> We don't allow to unlink it since it is crucial for binderfs to be useable
> but if we allow to rename it we make the unlink trivial to bypass. So
> prevent renaming too and simply treat the control dentry as immutable.
>
> Take the opportunity and turn the check for the control dentry into a
> separate helper is_binderfs_control_device() since it's now used in two
> places.
> Additionally, replace the custom rename dance we did with call to
> simple_rename().
Umm...
> +static inline bool is_binderfs_control_device(const struct inode *inode,
> + const struct dentry *dentry)
> +{
> + return BINDERFS_I(inode)->control_dentry == dentry;
> +}
What do you need an inode for?
static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
{
return inode->i_sb->s_fs_info;
}
so it looks like all you care about is the superblock. Which can be
had simply as dentry->d_sb...
Besides, what's the point of calling is_binderfs_device() in ->rename()?
If your directory methods are given dentries from another filesystem,
the kernel is already FUBAR. So your rename should simply do
if (is_binderfs_control_device(old_dentry) ||
is_binderfs_control_device(new_dentry))
return -EPERM;
return simple_rename(..);
and that's it...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] binder: fix use-after-free due to ksys_close() during fdget()
On Fri, Dec 14, 2018 at 12:38:15PM -0800, Todd Kjos wrote:
> 44d8047f1d8 ("binder: use standard functions to allocate fds")
> exposed a pre-existing issue in the binder driver.
>
> fdget() is used in ksys_ioctl() as a performance optimization.
> One of the rules associated with fdget() is that ksys_close() must
> not be called between the fdget() and the fdput(). There is a case
> where this requirement is not met in the binder driver which results
> in the reference count dropping to 0 when the device is still in
> use. This can result in use-after-free or other issues.
>
> If userpace has passed a file-descriptor for the binder driver using
> a BINDER_TYPE_FDA object, then kys_close() is called on it when
> handling a binder_ioctl(BC_FREE_BUFFER) command. This violates
> the assumptions for using fdget().
>
> The problem is fixed by deferring the fd close using task_work_add()
> so ksys_close() is called after returning from binder_ioctl().
>
> Fixes: 44d8047f1d87a ("binder: use standard functions to allocate fds")
> Suggested-by: Al Viro
> Signed-off-by: Todd Kjos
Umm... IMO you are making it more brittle than needed. Descriptors
(as in, integers serving as indices in file descriptor tables) are
sensitive to a lot of things and generally you don't want to pass
them around, at least not without a lot more context than you do.
References to struct file are much more robust. And frankly, ksys_close()
is best not touched - what you want is basically a version of __close_fd()
that would grab a reference to struct file before filp_close() and
passed that reference to you. Then your delayed ksys_close() would turn
into (equally delayed) fput().
Something like
int rip_fd(int fd, struct file **res)
{
struct files_struct *files = current->files;
struct file *file;
struct fdtable *fdt;
spin_lock(&files->file_lock);
fdt = files_fdtable(files);
if (fd >= fdt->max_fds)
goto out_unlock;
file = fdt->fd[fd];
if (!file)
goto out_unlock;
rcu_assign_pointer(fdt->fd[fd], NULL);
__put_unused_fd(files, fd);
spin_unlock(&files->file_lock);
get_file(file);
*res = file;
return filp_close(file, files);
out_unlock:
spin_unlock(&files->file_lock);
*res = NULL;
return -ENOENT;
}
used as
error = rip_fd(fd, &file);
/* we are committed to arranging fput(file) now, error or not */
Frankly, the main objection to exporting that would be to avoid encouraging
the "we'll just pass the number around" kind of braindamage. In case of
binder you are locked into that braindamage by an atrocious userland API
design and warnings along the lines of "use that and you *will* have
to explain yourself; yes, we will be watching" might suffice to prevent
additional uses...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization
On Wed, Dec 05, 2018 at 04:21:55PM -0800, Todd Kjos wrote: > > How about grabbing the references to all victims (*before* screwing with > > ksys_close()), sticking them into a structure with embedded callback_head > > and using task_work_add() on it, the callback doing those fput()? > > > > The callback would trigger before the return to userland, so observable > > timing of the final close wouldn't be changed. And it would avoid the > > kludges like this. > > I'll rework it according to your suggestion. I had hoped to do this in a way > that doesn't require adding calls to non-exported functions since we are > trying to clean up binder (I hear you snickering) to be a better citizen and > not rely on internal functions that drivers shouldn't be using. I presume > there are no plans to export task_work_add()... Er... Your variant critically depends upon binder being non-modular; if it *was* built as a module, you could * lose the timeslice just after your fput() * have another process hit the final fput() *and* close the struct file * now that module refcount is not pinned by anything, get rmmod remove your module * have the process in binder_ioctl() regain the timeslice and find the code under it gone. That's one of the reasons why such kludges are brittle as hell - normally you are guaranteed that once fdget() has succeeded, the final fput() won't happen until fdput(). With everything that guarantees in terms of code/data not going away under you. This patch relies upon the lack of accesses to anything sensitive after that fput() added into binder_ioctl(). Which is actually true, but only because the driver is not modular... At least this variant (task_work_add()-based) doesn't depend on anything subtle - the lack of exports is the only problem there (IOW, it would've worked in a module if not for that). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] binder: fix use-after-free due to fdget() optimization
On Wed, Dec 05, 2018 at 01:16:01PM -0800, Todd Kjos wrote:
> 44d8047f1d87a ("binder: use standard functions to allocate fds")
> exposed a pre-existing issue in the binder driver.
>
> fdget() is used in ksys_ioctl() as a performance optimization.
> One of the rules associated with fdget() is that ksys_close() must
> not be called between the fdget() and the fdput(). There is a case
> where this requirement is not met in the binder driver (and possibly
> other drivers) which results in the reference count dropping to 0
> when the device is still in use. This can result in use-after-free
> or other issues.
>
> This was observed with the following sequence of events:
>
> Task A and task B are connected via binder; task A has /dev/binder open at
> file descriptor number X. Both tasks are single-threaded.
>
> 1. task B sends a binder message with a file descriptor array
>(BINDER_TYPE_FDA) containing one file descriptor to task A
> 2. task A reads the binder message with the translated file
>descriptor number Y
> 3. task A uses dup2(X, Y) to overwrite file descriptor Y with
>the /dev/binder file
> 4. task A unmaps the userspace binder memory mapping; the reference
>count on task A's /dev/binder is now 2
> 5. task A closes file descriptor X; the reference count on task
>A's /dev/binder is now 1
> 6. task A forks off a child, task C, duplicating the file descriptor
>table; the reference count on task A's /dev/binder is now 2
> 7. task A invokes the BC_FREE_BUFFER command on file descriptor X
>to release the incoming binder message
> 8. fdget() in ksys_ioctl() suppresses the reference count increment,
>since the file descriptor table is not shared
> 9. the BC_FREE_BUFFER handler removes the file descriptor table
>entry for X and decrements the reference count of task A's
>/dev/binder file to 1
> 10.task C calls close(X), which drops the reference count of
>task A's /dev/binder to 0 and frees it
> 11.task A continues processing of the ioctl and accesses some
>property of e.g. the binder_proc => KASAN-detectable UAF
>
> Fixed by using get_file() / fput() in binder_ioctl().
Note that this patch does *not* remove the nasty trap caused by the garbage
in question - struct file can be freed before we even return from
->unlocked_ioctl(). Could you describe in details the desired behaviour
of this interface?
How about grabbing the references to all victims (*before* screwing with
ksys_close()), sticking them into a structure with embedded callback_head
and using task_work_add() on it, the callback doing those fput()?
The callback would trigger before the return to userland, so observable
timing of the final close wouldn't be changed. And it would avoid the
kludges like this.
Of course, the proper fix would require TARDIS and set of instruments for
treating severe case of retrocranial inversion, so that this "ABI" would've
never existed, but...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote: > Acked-by: Darren Hart (VMware) > > As for a longer term solution, would it be possible to init fops in such > a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg > so we don't have to duplicate this boilerplate for every ioctl fops > structure? Bad idea, that... Because several years down the road somebody will add an ioctl that takes an unsigned int for argument. Without so much as looking at your magical mystery macro being used to initialize file_operations. FWIW, I would name that helper in more blunt way - something like compat_ioctl_only_compat_pointer_ioctls_here()... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote:
> From: Samuel Thibault
>
> From: Samuel Thibault
>
> If softsynthx_read() is called with `count < 3`, `count - 3` wraps, causing
> the loop to copy as much data as available to the provided buffer. If
> softsynthx_read() is invoked through sys_splice(), this causes an
> unbounded kernel write; but even when userspace just reads from it
> normally, a small size could cause userspace crashes.
Or you could try this (completely untested, though):
diff --git a/drivers/staging/speakup/speakup_soft.c
b/drivers/staging/speakup/speakup_soft.c
index a61bc41b82d7..198936ed0b54 100644
--- a/drivers/staging/speakup/speakup_soft.c
+++ b/drivers/staging/speakup/speakup_soft.c
@@ -192,13 +192,10 @@ static int softsynth_close(struct inode *inode, struct
file *fp)
return 0;
}
-static ssize_t softsynthx_read(struct file *fp, char __user *buf, size_t count,
- loff_t *pos, int unicode)
+static ssize_t softsynthx_read_iter(struct kiocb *iocb, struct iov_iter *to,
int unicode)
{
int chars_sent = 0;
- char __user *cp;
char *init;
- u16 ch;
int empty;
unsigned long flags;
DEFINE_WAIT(wait);
@@ -211,7 +208,7 @@ static ssize_t softsynthx_read(struct file *fp, char __user
*buf, size_t count,
if (!synth_buffer_empty() || speakup_info.flushing)
break;
spin_unlock_irqrestore(&speakup_info.spinlock, flags);
- if (fp->f_flags & O_NONBLOCK) {
+ if (iocb->ki_flags & IOCB_NOWAIT) {
finish_wait(&speakup_event, &wait);
return -EAGAIN;
}
@@ -224,11 +221,14 @@ static ssize_t softsynthx_read(struct file *fp, char
__user *buf, size_t count,
}
finish_wait(&speakup_event, &wait);
- cp = buf;
init = get_initstring();
/* Keep 3 bytes available for a 16bit UTF-8-encoded character */
- while (chars_sent <= count - 3) {
+ while (iov_iter_count(to)) {
+ u_char s[3];
+ int l, n;
+ u16 ch;
+
if (speakup_info.flushing) {
speakup_info.flushing = 0;
ch = '\x18';
@@ -244,60 +244,47 @@ static ssize_t softsynthx_read(struct file *fp, char
__user *buf, size_t count,
spin_unlock_irqrestore(&speakup_info.spinlock, flags);
if ((!unicode && ch < 0x100) || (unicode && ch < 0x80)) {
- u_char c = ch;
-
- if (copy_to_user(cp, &c, 1))
- return -EFAULT;
-
- chars_sent++;
- cp++;
+ s[0] = ch;
+ l = 1;
} else if (unicode && ch < 0x800) {
- u_char s[2] = {
- 0xc0 | (ch >> 6),
- 0x80 | (ch & 0x3f)
- };
-
- if (copy_to_user(cp, s, sizeof(s)))
- return -EFAULT;
-
- chars_sent += sizeof(s);
- cp += sizeof(s);
+ s[0] = 0xc0 | (ch >> 6);
+ s[1] = 0x80 | (ch & 0x3f);
+ l = 2;
} else if (unicode) {
- u_char s[3] = {
- 0xe0 | (ch >> 12),
- 0x80 | ((ch >> 6) & 0x3f),
- 0x80 | (ch & 0x3f)
- };
-
- if (copy_to_user(cp, s, sizeof(s)))
- return -EFAULT;
-
- chars_sent += sizeof(s);
- cp += sizeof(s);
+ s[0] = 0xe0 | (ch >> 12);
+ s[1] = 0x80 | ((ch >> 6) & 0x3f);
+ s[2] = 0x80 | (ch & 0x3f);
+ l = 3;
}
-
+ n = copy_to_iter(s, l, to);
+ if (n != l) {
+ iov_iter_revert(to, n);
+ spin_lock_irqsave(&speakup_info.spinlock, flags);
+ break;
+ }
+ chars_sent += l;
spin_lock_irqsave(&speakup_info.spinlock, flags);
}
- *pos += chars_sent;
+ iocb->ki_pos += chars_sent;
empty = synth_buffer_empty();
spin_unlock_irqrestore(&speakup_info.spinlock, flags);
if (empty) {
speakup_start_ttys();
- *pos = 0;
+ iocb->ki_pos = 0;
}
return chars_sent;
}
-static ssize_t softsynth_read(struct file *fp, char __user *buf, size_t count,
+static ssize_t softsynth_read_iter(struct kiocb *iocb, struct iov_iter *to)
loff_t *pos)
{
- return softsynthx_read(fp, buf, count
Re: simplify procfs code for seq_file instances V3
On Wed, May 16, 2018 at 11:43:04AM +0200, Christoph Hellwig wrote: > We currently have hundreds of proc files that implement plain, read-only > seq_file based interfaces. This series consolidates them using new > procfs helpers that take the seq_operations or simple show callback > directly. > > A git tree is available at: > > git://git.infradead.org/users/hch/misc.git proc_create.3 Pulled, but the last bit is a bleeding atrocity in need of followup cleanup. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: simplify procfs code for seq_file instances V2
On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote:
> +++ b/fs/proc/internal.h
> @@ -48,8 +48,8 @@ struct proc_dir_entry {
> const struct seq_operations *seq_ops;
> int (*single_show)(struct seq_file *, void *);
> };
> - unsigned int state_size;
> void *data;
> + unsigned int state_size;
> unsigned int low_ino;
> nlink_t nlink;
> kuid_t uid;
Makes sense
> @@ -62,9 +62,9 @@ struct proc_dir_entry {
> umode_t mode;
> u8 namelen;
> #ifdef CONFIG_64BIT
> -#define SIZEOF_PDE_INLINE_NAME (192-139)
> +#define SIZEOF_PDE_INLINE_NAME (192-155)
> #else
> -#define SIZEOF_PDE_INLINE_NAME (128-87)
> +#define SIZEOF_PDE_INLINE_NAME (128-95)
> #endif
> char inline_name[SIZEOF_PDE_INLINE_NAME];
> } __randomize_layout;
*UGH*
Both to the original state and that kind of "adjustments".
Incidentally, with __bugger_layout in there these expressions
are simply wrong.
If nothing else, I would suggest turning the last one into
char inline_name[];
in hope that layout won't get... randomized that much and
used
#ifdef CONFIG_64BIT
#define PDE_SIZE 192
#else
#define PDE_SIZE 128
#endif
union __proc_dir_entry {
char pad[PDE_SIZE];
struct proc_dir_entry real;
};
#define SIZEOF_PDE_INLINE_NAME (PDE_SIZE - offsetof(struct proc_dir_entry,
inline_name))
for constants, adjusted sizeof and sizeof_field when creating
proc_dir_entry_cache and turned proc_root into
union __proc_dir_entry __proc_root = { .real = {
.low_ino= PROC_ROOT_INO,
.namelen= 5,
.mode = S_IFDIR | S_IRUGO | S_IXUGO,
.nlink = 2,
.refcnt = REFCOUNT_INIT(1),
.proc_iops = &proc_root_inode_operations,
.proc_fops = &proc_root_operations,
.parent = &__proc_root.real,
.subdir = RB_ROOT,
.name = __proc_root.real.inline_name,
.inline_name= "/proc",
}};
#define proc_root __proc_root.real
(or actually used __proc_root.real in all of a 6 places where it remains).
> diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> index baf1994289ce..7d94fa005b0d 100644
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -40,7 +40,7 @@ static struct net *get_proc_net(const struct inode *inode)
>
> static int seq_open_net(struct inode *inode, struct file *file)
> {
> - size_t state_size = PDE(inode)->state_size;
> + unsigned int state_size = PDE(inode)->state_size;
> struct seq_net_private *p;
> struct net *net;
You and your "size_t is evil" crusade...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: KASAN: use-after-free Read in remove_wait_queue
On Mon, Feb 12, 2018 at 06:11:02PM +0100, Dmitry Vyukov wrote: > The commit on which it was triggered already includes this fix. So > there must be another bug. Any chance of bisecting it? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL
On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> Removed '(' from the end of line, coding style issue.
The one and only reason for warnings is that they point to
places more likely to be dodgy. There is no inherent value
in having e.g. checkpatch.pl STFU, all wanking about uniformity
of style nonwithstanding.
If you end up figuring out how to manipulate some code so that
some warning would go away, something is wrong. Either the
warning is bogus (which it might very well be - the point of
a warning is "might be worth looking here" and considering
the... level of sophistication of some of those, you can't
expect to get no false alarms) or there *is* something
dodgy in the vicinity. Dodgy beyond the "this heuristic
triggers", that is.
The first thing to do when dealing with any warning is to
look around and see if it has caught something interesting.
In this case the warning points to excessively long expressions.
With your patch they *still* are just as long, but split in
more unnatural way, making checkpatch.pl miss them.
All of them appear to have the same form - CPHYSADDR applied to
nlm_mmio_base result. Moreover, looking around that file
shows that all uses of CPHYSADDR and nlm_mmio_base in it
are parts of such expressions.
Most of those expressions are too long and hard to read, so
cleaning them up would probably be a good idea. And seeing
that composition of these functions keeps occuring in that
sucker, making that composition an inlined helper appears
to be called for.
So
static inline something( offset)
{
return CPHYSADDR(nlm_mmio_base(offset));
}
and turn those into
gmac4_addr = ioremap(something(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);
void __iomem *gmac0_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET),
0xfff);
gpio_addr = ioremap(something(NETLOGIC_IO_GPIO_OFFSET), 0xfff);
ndata0.mii_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET), 0xfff);
and turn
res->start = CPHYSADDR(nlm_mmio_base(offset));
in xlr_resource_init() into
res->start = something(offset);
as well. What should the types be? Result is either fed to ioremap(), or
stored in struct resource 'start' field. The latter is resource_size_t;
the former varies among the architectures. Some take resource_size_t,
some - phys_addr_t, some - unsigned long. On mips (that driver appears to
be mips-only) ioremap takes phys_addr_t; the difference is cosmetical,
anyway, since resource_size_t is typedefed to phys_addr_t on all architectures.
So let it return phys_addr_t. As for the argument type, looks like it's
an explicit constant or, in case of xlr_resource_init(), an int. Grepping
shows that all constants involved do fit into int, so we arrive at
static inline phys_addr_t something(int offset)
{
return CPHYSADDR(nlm_mmio_base(offset));
}
probably best placed just before the first use. OTOH, nlm_mmio_base() is
declared as taking uint32_t, so that might be a better fit for argument.
All constants fed there are between 0 and 2^31, so the only question is
what values does xlr_resource_init() get passed in 'offset' argument.
Callers are
xlr_resource_init(&xlr_net1_res[mac * 2],
xlr_gmac_offsets[mac + 4],
xlr_gmac_irqs[mac + 4]);
xlr_resource_init(&xlr_net0_res[0], xlr_gmac_offsets[0],
xlr_gmac_irqs[0]);
xlr_resource_init(&xlr_net0_res[mac * 2],
xlr_gmac_offsets[mac],
xlr_gmac_irqs[mac]);
xlr_resource_init(&xlr_net0_res[mac * 2], xlr_gmac_offsets[mac],
xlr_gmac_irqs[mac]);
so in all cases we have xlr_gmac_offsets[] passed. What's
going on with that array? Initialized as
static u32 xlr_gmac_offsets[] = {
NETLOGIC_IO_GMAC_0_OFFSET, NETLOGIC_IO_GMAC_1_OFFSET,
NETLOGIC_IO_GMAC_2_OFFSET, NETLOGIC_IO_GMAC_3_OFFSET,
NETLOGIC_IO_GMAC_4_OFFSET, NETLOGIC_IO_GMAC_5_OFFSET,
NETLOGIC_IO_GMAC_6_OFFSET, NETLOGIC_IO_GMAC_7_OFFSET
};
and never modified, apparently. OK, that pretty much settles it -
xlr_resource_init() should be getting u32 instead of int. It's harmless
(the constants here are in the same range), but the things will be easier
for reader that way:
static inline phys_addr_t something(u32 offset)
{
return CPHYSADDR(nlm_mmio_base(offset));
}
static void xlr_resource_init(struct resource *res, u32 offset, int irq)
{
res->name = "gmac";
res->start = something(offset);
While we are at it, 'irq' argument appears to be always taken from
xlr_gmac_irqs[], which is also u32. So I'd probably do
static void xlr_resource_init(struct resource *res, u32 offset, u32 irq)
instead.
Now, what do we call that helper? Definition of CPHYSADDR is
/*
* Returns the physical
Re: [PATCH v2] binder: fix proc->files use-after-free
On Thu, Nov 16, 2017 at 09:56:50AM -0800, Todd Kjos wrote:
> +static struct files_struct *binder_get_files_struct(struct binder_proc *proc)
> +{
> + return get_files_struct(proc->tsk);
> +}
Hell, _no_. You should never, ever use the result of get_files_struct() for
write access. It's strictly for "let me look at other process' descriptor
table". The whole reason for proc->files is that we want to keep a reference
that *could* be used for modifying the damn thing. And such can be obtained
only by the process itself.
The rules are:
* you can use current->files both for read and write
* you can use get_files_struct(current) to get a reference that
can be used for modifying the damn thing. Then it can be passed to
any other process and used by it.
* you can use get_files_struct(some_other_task) to get a reference
that can be used for examining the descriptor table of that other task.
Violation of those rules means an exploitable race. Here's the logics
fdget() and friends are based on: "I'm going to do something to file
refered by descriptor N. If my descriptor table is not shared, all
struct file references in it will stay around - I'm not changing it,
nobody else has it as their ->current, so any additional references
to that descriptor table will *not* be used for modifying it.
In other words, I don't need to bump the refcount of struct file I'm
about to work with - the reference from my descriptor table will keep
it alive".
Your patch breaks those assumptions. NAK.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: fix missing break in switch statement.
On Thu, Nov 09, 2017 at 05:19:55PM +, Colin King wrote: > From: Colin Ian King > > The PI433_IOC_WR_RX_CFG case is missing a break and will fall through > to the default case and errorenously return -EINVAL. Fix this by > adding in missing break. Folded and pushed... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code
On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote:
> Thanks for testing it!
>
> That means we did not copy any data and the kernel continues with
> an uninitialized buffer, right? The problem may be the definition of
>
> struct kib_immediate_msg {
> struct lnet_hdr ibim_hdr;/* portals header */
> char ibim_payload[0]; /* piggy-backed payload */
> } WIRE_ATTR;
>
> The check that Al added will try to ensure that we don't write
> beyond the size of the ibim_payload[] array, which unfortunately
> is defined as a zero-byte array, so I can see why it will now
> fail. However, it's already broken in mainline now, with or without
> my patch.
>
> Are you able to come up with a fix that avoids the warning in
> 'allmodconfig' and makes the function do something reasonable
> again?
Might make sense to try and use valid C99 for "array of indefinite
size as the last member", i.e.
struct kib_immediate_msg {
struct lnet_hdr ibim_hdr;/* portals header */
char ibim_payload[]; /* piggy-backed payload */
} WIRE_ATTR;
Zero-sized array as the last member is gcc hack predating that;
looks like gcc gets confused into deciding that it knows the distance
from the end of object...
Said that, are we really guaranteed the IBLND_MSG_SIZE bytes
in there?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning
On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote: > > MILD SUGGESTION: don't spell the function name out in format strings; > > "this_function: foo is %d", n > > might be better off as > > "%s: foo is %d", __func__, n > > in case you ever move it to another function or rename your function. > > Thank you sir, may I have another. > > checkpatch messages are single line. Too bad... Incidentally, being able to get more detailed explanation of a warning might be a serious improvement, especially if it contains the rationale. Hell, something like TeX handling of errors might be a good idea - warning printed, offered actions include 'give more help', 'continue', 'exit', 'from now on suppress this kind of warning', 'from now on just dump this kind of warning into log and keep going', 'from now on dump all warnings into log and keep going'. And yes, I'm serious about having something like "mild suggestion" as possible severity - people are using that thing to look for potential improvements to make and 'such and such change might be useful for such and such reasons' is a lot more useful than 'this needs to be thus because it must be thus or I'll keep warning'. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning
On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote: > On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote: > > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote: > > > Replace string with formatted arguments in the dev_warn() call. It removes > > > the checkpatch warning: > > > > > > WARNING: Prefer using "%s", __func__ to embedded function names > [] > > Again, checkpatch warning is badly written > > In your opinion, what wording would be better? MILD SUGGESTION: don't spell the function name out in format strings; "this_function: foo is %d", n might be better off as "%s: foo is %d", __func__, n in case you ever move it to another function or rename your function. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning
On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
>
> WARNING: Prefer using "%s", __func__ to embedded function names
> #417: FILE: main_usb.c:417:
> +"usb_device_reset fail status=%d\n", status);
>
> total: 0 errors, 1 warnings, 1058 lines checked
>
> And after fix:
>
> main_usb.c has no obvious style problems and is ready for submission.
> - "usb_device_reset fail status=%d\n", status);
> + "%s=%d\n", "usb_device_reset fail status", status);
Would you mind explaining the meaning of that warning? Incidentally, why not
"%se_reset fail status=%d\n", "usb_devic", status);
- it would produce the identical output and silences checkpatch even more
reliably. Discuss.
Sarcasm aside, when you are proposing a change, there are several questions you
really must ask yourself and be able to answer. First and foremost, of course,
is
* Why is it an improvement?
If the answer to that was "it makes a warning go away", the next questions are
* What are we warned about?
* What is problematic in the original variant?
* Is the replacement free from the problem we had in the original?
and if the answer to any of that is "I don't know", the next step is _not_
"send it anyway". "Try to figure out" might be a good idea, with usual
heuristics regarding the reading comprehension ("if a sentence remains
a mystery, see if there are any unfamiliar words skipped at the first reading
and find what they mean", etc.) applying.
In this particular case the part you've apparently skipped was, indeed,
critical - "__func__". I am not trying to defend the quality of checkpatch -
the warning is badly worded, the tool is badly written and more often than
not its suggestions reflect nothing but authors' arbitrary preferences.
This one, however, does have some rationale behind it. Namely, if some
debugging output contains the name of a function that has produced it,
having that name spelled out in format string is not a good idea. If
that code gets moved elsewhere one would have to replace the name in format
string or face confusing messages refering to the function where that
code used to be. Since __func__ is interpreted as a constant string
initialized with the name of the function it's used in, it is possible
to avoid spelling the function name out - instead of
"my_function: pink elephants; reduce the daily intake to %d glasses\n", n
one could use
"%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
producing the same output and avoiding the need to adjust anything when
the whole thing is moved to another function. It's not particularly big
deal, but it's a more or less reasonable suggestion.
Your change, of course, has achieved only one thing - it has moved the
explicitly spelled out function name out of format string. It's still
just as explicitly spelled out, still would need to be adjusted if moved
to another function and the whole thing has become harder to read and
understand since you've buried other parts of message in the place where
it's harder to follow.
Again, checkpatch warning is badly written, but the main problem with
your posting is not that you had been confused by checkpatch - it's
that you have posted it based on an incomprehensible message with no better
rationale than "it shuts checkpatch up, dunno why and what about".
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: android: Replace strcpy with strlcpy
On Sat, Mar 11, 2017 at 09:47:30PM +0100, Julia Lawall wrote: > > > On Sun, 12 Mar 2017, simran singhal wrote: > > > Replace strcpy with strlcpy as strcpy does not check for buffer > > overflow. > > This is found using Flawfinder. > > > > Signed-off-by: simran singhal > > --- > > drivers/staging/android/ashmem.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/android/ashmem.c > > b/drivers/staging/android/ashmem.c > > index 7cbad0d..eb2f4ef 100644 > > --- a/drivers/staging/android/ashmem.c > > +++ b/drivers/staging/android/ashmem.c > > @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void > > __user *name) > > if (unlikely(asma->file)) > > ret = -EINVAL; > > else > > - strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name); > > + strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name, > > + sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN)); > > There is a parenthesis in the wrong place. Worse - moving parenthesis to just after asma->name would result in interestingly bogus value (size + amount skipped instead of size - amount skipped). Folks, blind changes in name of security are seriously counterproductive; fortunately, in this particular case overflow prevention is taken care of by earlier code (source of strcpy is a local array of size that isn't enough to cause trouble and it is NUL-terminated), so that particular strlcpy() is simply pointless, but if not for that... Variant with sizeof(asma->name) + ASHMEM_NAME_PREFIX_LEN would've invited an overflow *and* made it harder to spot in the future - "it uses strlcpy, no worries about overflows here"... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: Replace strcpy with strlcpy
On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> Replace strcpy with strlcpy as strcpy does not check for buffer
> overflow.
> This is found using Flawfinder.
>
> Signed-off-by: simran singhal
> ---
> drivers/staging/android/ashmem.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/ashmem.c
> b/drivers/staging/android/ashmem.c
> index 7cbad0d..eb2f4ef 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user
> *name)
> if (unlikely(asma->file))
> ret = -EINVAL;
> else
> - strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> + strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> + sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
Trivial C quiz: given
struct ashmem_area {
char name[ASHMEM_FULL_NAME_LEN];
struct list_head unpinned_list;
struct file *file;
size_t size;
unsigned long prot_mask;
};
static int set_name(struct ashmem_area *asma, void __user *name)
what, in your opinion, would be
1) type of asma->name
2) type of asma->name + ASHMEM_NAME_PREFIX_LEN
3) value of sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN)
As a bonus question,
4) what is the value of this kind of patches?
1) NFUZRZ_SHYY_ANZR_YRA-ryrzrag neenl bs pune
2) cbvagre gb pune
3) fvmr bs n cbvagre
4) fbpvbybtvpny - ernql-znqr vyyhfgengvbaf bs crevyf bs
pnetb phyg.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH]staging: fbtft: Fix sparse warnings about endianness
On Tue, Feb 14, 2017 at 09:18:25AM -0800, Greg KH wrote:
> On Tue, Feb 14, 2017 at 04:53:10PM +0800, maomao xu wrote:
> > Fixed sparse warnings about endianness. E.g.:
> >
> > warning: incorrect type in assignment (different base types)
>
> Why are these lines indented?
>
> > Signed-off-by: maomao xu
> >
> > diff --git a/drivers/staging/fbtft/fbtft-io.c
> > b/drivers/staging/fbtft/fbtft-io.c
> > index d868405..ffb9a3b 100644
> > --- a/drivers/staging/fbtft/fbtft-io.c
> > +++ b/drivers/staging/fbtft/fbtft-io.c
> > @@ -71,7 +71,7 @@ int fbtft_write_spi_emulate_9(struct fbtft_par *par, void
> > *buf, size_t len)
> > src++;
> > }
> > tmp |= ((*src & 0x0100) ? 1 : 0);
> > - *(u64 *)dst = cpu_to_be64(tmp);
> > + *(__be64 *)dst = cpu_to_be64(tmp);
>
> Really? That seems very odd. I need a maintainer's ack for this before
> I can take it...
If anything, I would turn the inner loop into
tmp = 0;
for (j = 0; j < 7; j++) {
tmp <<= 9;
tmp |= *src++ & 0x1ff;
}
tmp <<= 1;
there - the whole thing looks like an obfuscated "take an array of 9-bit
values, each stored in host-endian 16bit, repack them into octets".
Which architecture is that supposed to run on? Anything that doesn't like
unaligned stores won't be happy with that code...
Another interesting part is that we have size = len / 2 and verify that len
is a multiple of 8. I.e. that size is only guaranteed to be a multiple
of 4. Each pass through the outer loop consumes 8 16bit values and decrements
size by 8, so if len is e.g. 24 we'll end up accepting that, get size equal
to 12, pass through the outer loop twice and chew through 32 bytes of buf...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv4 2/8] Fixed variables not being consistently lower case
On Sun, Dec 25, 2016 at 10:34:54PM +, Jonathan Cameron wrote:
>
>
> On 25 December 2016 20:14:09 GMT+00:00, Al Viro
> wrote:
> >On Sun, Dec 25, 2016 at 01:41:06PM -0600, Scott Matheina wrote:
> >> Across the file, variables were sometimes upper case, some times
> >> lower case, this fix addresses a few of the instances with this
> >> inconsistency.
> >
> >NAK. Go learn C and don't come back until you've done that. If
> >somebody
> >has told you that you can contribute without knowing the language,
> >they'd
> >lied - you really can't. And I would *STRONGLY* recommend to stay away
> >from drivers/staging while you are learning C - it's like trying to use
> >a public restroom wall as a sex-ed textbook.
>
> Please can we keep things polite.
In case you have not realized it,
* Scott is -><- that close to joining the select group of kooks who
get filtered out within days of getting a new account. At this point he
probably still can spend a while learning what needs to be learnt and
reappear with less inane patches after a while, with everything getting
forgotten. A week more of the same kind of postings will almost certainly
cement the reputation of clue-resistant noise source beyond any chance of
repair.
* I am 100% sure that he has not learnt C - hadn't even started doing
so. This, BTW, is not a value judgement of any kind; it's just a highly
likely conclusion based on what he'd been posting. This particular patch,
for example, changes identifiers in a bunch of places, none of which happens
to be a declaration. The reasons why that cannot be correct become obvious
to anyone who had opened _any_ textbook after a few pages. Or tried to
write and run any program in C. Again, it's not "should" - it's really one
of the first things that gets learnt ("you need to declare all variables
explicitly and the names are case-sensitive").
* One really cannot produce any useful patches without understanding
the language. As this (and previous) patch series sent by Scott has
demonstrated, BTW. Learning C is a hard prerequisite for doing any development
in a project written in C, including the kernel. Fortunately, it is not
hard to do.
* Trying to do that purely by osmosis is not the best way, but it's
doable. HOWEVER, trying to do that by osmosis when swimming in drivers/staging
is really on par with using a WC wall as sex ed tutorial. Results will be
awkward, painful and won't satisfy anybody. Very odd ideas of how the things
work are also practically guaranteed.
drivers/staging can serve as a playground for learning how to
produce patches, but only if you already know the language. Otherwise
it really, really must be avoided - it's full of unidiomatic and very
badly written C. It serves as quarantine barracks for much more serious
reasons than anything checkpatch.pl is capable of picking. Directing the
folks new to kernel development towards that pile of code is not particularly
kind, but potentially useful. Doing that to those who are still learning C
goes far beyond "not kind". I can't stress it enough - don't go there if
you are still learning the language.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv4 2/8] Fixed variables not being consistently lower case
On Sun, Dec 25, 2016 at 01:41:06PM -0600, Scott Matheina wrote: > Across the file, variables were sometimes upper case, some times > lower case, this fix addresses a few of the instances with this > inconsistency. NAK. Go learn C and don't come back until you've done that. If somebody has told you that you can contribute without knowing the language, they'd lied - you really can't. And I would *STRONGLY* recommend to stay away from drivers/staging while you are learning C - it's like trying to use a public restroom wall as a sex-ed textbook. While we are at it, it might be a good idea to check if the kernel builds after your changes and see what errors are produced. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/8] Sparse warning fixes in Lustre.
On Wed, Dec 07, 2016 at 05:41:26PM -0500, Oleg Drokin wrote: > This set of fixes aims at sparse warnings. Speaking of the stuff sparse catches there: class_process_proc_param(). I've tried to describe what I think of that Fine Piece Of Software several times, but I had to give up - my command of obscenity is not up to the task, neither in English nor in Russian. Please, take it out. Preferably - along with the ->ldo_process_config()/->process_config() thing. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC 0/6] Module for tracking/accounting shared memory buffers
On Tue, Oct 11, 2016 at 04:50:04PM -0700, Ruchi Kandoi wrote:
> memtrack maintains a per-process list of shared buffer references, which is
> exported to userspace as /proc/[pid]/memtrack. Buffers can be optionally
> "tagged" with a short string: for example, Android userspace would use this
> tag to identify whether buffers were allocated on behalf of the camera stack,
> GL, etc. memtrack also exports the VMAs associated with these buffers so
> that pages already included in the process's mm counters aren't
> double-counted.
>
> Shared-buffer allocators can hook into memtrack by embedding
> struct memtrack_buffer in their buffer metadata, calling
> memtrack_buffer_{init,remove} at buffer allocation and free time, and
> memtrack_buffer_{install,uninstall} when a userspace process takes or
> drops a reference to the buffer. For fd-backed buffers like dma-bufs, hooks
> in
> fdtable.c and fork.c automatically notify memtrack when references are added
> or
> removed from a process's fd table.
>
> This patchstack adds memtrack hooks into dma-buf and ion. If there's upstream
> interest in memtrack, it can be extended to other memory allocators as well,
> such as GEM implementations.
No, with a side of Hell, No. Not to mention anything else,
* descriptor tables do not belong to any specific task_struct and
actions done by one show up in all who share that thing.
* shared descriptor table does not imply belonging to the same
group.
* shared descriptor table can become unshared at any point, invisibly
for that Fine Piece Of Software.
* while we are at it, blocking allocation under several spinlocks
(and with interrupts disabled, for good measure) is generally considered
a bloody bad idea.
That - just from the quick look through that patchset. Bringing task_struct
into the API is already sufficient for a NAK.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging/ion: Add support to get ion handle from dma buf
On Wed, Jan 06, 2016 at 10:21:33AM -0800, Linus Torvalds wrote: > On Wed, Jan 6, 2016 at 1:06 AM, Dan Carpenter > wrote: > > It's not really necessary to CC linux-kernel. No one reads it. Some of us still do. > > I only send patches there when there isn't another public mailing > > list available. > > Actually, cc'ing lkml is still often a good idea, because it's a great > archiving point. *nod* ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: add __iomem for io_base and registers
On Sat, Jan 02, 2016 at 08:30:21PM +, Hugo Camboulive wrote: > This removes a few Sparse warnings. > + g.dim2 = (struct dim2_regs __iomem *)dim_base_address; > -u8 dim_startup(void *dim_base_address, u32 mlb_clock); > +u8 dim_startup(void __iomem *dim_base_address, u32 mlb_clock); Umm... Why not have it take struct dim2_regs __iomem * as argument, and to hell with that cast? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rtl8723au: Fix sparse errors in rtl8723a_cmd.c
On Tue, Oct 06, 2015 at 12:32:28AM -0400, Jacob Kiefer wrote:
> int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
> {
> - *((u32 *)param) = cpu_to_le32(*((u32 *)param));
> + __le32 leparam;
>
> - FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
> + leparam = cpu_to_le32(*((u32 *)param));
> +
> + FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&leparam);
Why not fill the thing we are passing already with little-endian? There's
only one caller, after all...
> int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
> {
> u8 buf[5];
> + __le32 lemask;
>
> memset(buf, 0, 5);
> - mask = cpu_to_le32(mask);
> - memcpy(buf, &mask, 4);
> + lemask = cpu_to_le32(mask);
> + memcpy(buf, &lemask, 4);
> buf[4] = arg;
>
> FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
Ugh...
struct macid_config_eid {__le32 mask; u8 arg;} buf = {
.mask = cpu_to_le32(mask), .arg = arg
};
FillH2CCmd(padapter, MACID_CONFIG_EID, 5, &buf);
Why bother with memcpy/memset/whatnot when all you are trying to do is to
initialize a temporary structure? And no, it's not going to have any gaps...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
use of opaque subject lines
On Mon, Feb 02, 2015 at 08:13:10PM +0100, Andreas Ruprecht wrote: > On a serious note: I do understand what you're getting at, I don't take > that personally (and I will send a v2 addressing the things above), but > honestly, this kind of answer might just be a real turn-off for other > people trying to get into kernel development... > > I don't want to start a whole new 'attitude in the kernel community' > discussion, but I can't just let this go like that, sorry. Just during the last 12 hours or so, I've seen the following l-k traffic: Subject: [PATCH] usb: host/sl811-hcd: fix sparse warning Subject: [PATCH] usb: gadget: function/f_sourcesink: fix sparse warning Subject: [PATCH] tty: vt/vt: fix sparse warning Subject: [PATCH] scsi: fix sparse warnings Subject: [PATCH] bfa: bfa_core: fix sparse warning Subject: [PATCH] scsi: fix sparse warning Subject: [PATCH] xen/acpi-processor: fix sparse warning Subject: [PATCH] scsi: initio: fix sparse warnings Subject: [PATCH] scsi: dc395x: fix sparse warning Subject: [PATCH] scsi: eata: fix sparse warning Subject: [PATCH] scsi: qla1280: fix sparse warnings Subject: [PATCH] scsi: ips: fix sparse warnings Subject: [PATCH] fbdev: via/via_clock: fix sparse warning Subject: [PATCH] usb: gadget: fix sparse warnings Subject: [PATCH] usb: gadget: fix sparse warnings Subject: [PATCH] usb: gadget: function/uvc_v4l2.c: fix sparse warnings Subject: [PATCH] xen-netback: fix sparse warning Subject: [PATCH] thermal: int340x: fix sparse warning Subject: [PATCH] vxge: fix sparse warning Subject: Re: [PATCH] xen-netback: fix sparse warning Subject: [PATCH] ixgbe: fix sparse warnings Subject: [PATCH] samsung-laptop: fix sparse warning Subject: [PATCH] x86: thinkpad_acpi.c: fix sparse warning Subject: [PATCH] Sony-laptop: fix sparse warning one of those is an ACK from maintainer, the rest are (AFAICS) all "make something static", all with rather poor commit messages and subjects. Not threaded, either (that would've somewhat mitigated one of the problems). Bad attitude? I'd say that _this_ is one. l-k is high-traffic list; postings like that mean extra strain on those who are trying at least to skim through it and they are not easy on those who are trying to read the commit logs. And it's trivial to mitigate - choose less opaque subjects, for starters. Because these are about as opaque as it gets: essentially, it's "sparse had drawn my attention to $FILE; I've fixed that problem by making it STFU". Readers would like more information to filter upon? Tough luck, open the damn mail and look into it. One after another, after another, to the tune of dozens per night. All with the commit messages pretty much of the form "$TOOL has produced $MESSAGE; with diff below it doesn't". It's physically impossible to read through all l-k traffic; the rate is too high for that. Is making it possible to do minimal triage too much to ask for? I'm not blaming anyone for going after the low-hanging fruit like that to get some experience on simple stuff, but as it is, it reinforces seriously bad habits while creating considerable PITA for list readers. And yes, we'd all done our share of lousy commit messages, but this pretty much teaches newbies to do that all the time. Sigh... Folks, please * Use descriptive subject lines at least somewhere in a thread. * Describe what the thing does, besides "makes $TOOL to STFU". * Remember, you don't fix warnings - you deal with the problem (if any) in the code those warnings had pointed to. And if there isn't a problem (i.e. the warning was a false positive) you might be annotating the code to tell the dumb tool what's going on there. Which is a different genre, and needs different mindset when reviewing (it's easy to obfuscate a _real_ problem away - away from the $TOOL's ability to spot it, that is). * Remember that said tools are nowhere near strong AI; the same warning can come from very different underlying problems. These warnigns are heuristics - such-and-such looking code has high chances of needing attention. Give at least some indication what the problem _is_ - e.g. the sparse warnings that has spawned this flood might come from 1) function really is used only in the file it's defined in, never intended to be used elsewhere, ought to be static to avoid namespace pollution 2) function is used elsewhere, and its uses elsewhere have explicit externs right at the point of use. Dangerous, since there's no practical way for compiler (gcc, sparse, lint, whatever) to verify that there's no type mismatch. Ought to have extern placed in some header included both by the file where it's defined and by all files where it's used, with externs at the point of use removed. Need to be careful about the choice of header and location in it. 3) function _is_ declared in a common header, but it's either not included by the file where it actually lives, *or* is included only on some architectures an
Re: [PATCH] staging: lustre: osc: Fix sparse warning about osc_init
On Mon, Feb 02, 2015 at 02:36:43PM +0100, Andreas Ruprecht wrote:
> When running sparse on the osc/ subdirectory, it shows the
> following warning:
>
> andreas@workbox:~/linux-next$ make C=1 M=drivers/staging/lustre/lustre/osc/
> [...]
> drivers/staging/lustre/lustre/osc/osc_request.c:3335:12: warning:
> symbol 'osc_init' was not declared. Should it be static?
> [...]
>
> As this is the module init function, it can (and should) be declared
> static.
>
> Signed-off-by: Andreas Ruprecht
For pity sake, folks, could you at least try to produce less meaningless
commit summaries? "Fix sparse warning" says nothing whatsoever about
what's getting done. The role of sparse in that is simple - it has
provided a tip to look at that declaration. That's all; it might as well
had been offered by a guy puking at the next stall in the loo after big
party. And no, sparse alone is not sufficient to prove that it could be
made static - it might be used somewhere else with explicit extern, its
declaration might've been in a header that didn't get included here, or
under slightly incorrect ifdefs, etc. Such things need to be investigated
manually, not that it was hard to do. OK, in this case the tip had panned
out; it can be made static (quick grep shows that) and it's better off
that way - less namespace pollution, makes life easier when doing analysis
of that code, etc.
_That_ is the point of this patch, not making the holy oracle shut the bleeding
fuck up. If you want to credit sparse (or that guy puking in the next stall,
or whatever had drawn your attention to the function in question) - great,
do so inside the commit message. _AFTER_ having described what the patch
does ("make osc_init() static" or equivalent thereof), please. Ideally -
with something like "it's never used outside of osc_request.c" after summary
line (strictly speaking, something that serves as module_init might very well
be called elsewhere in the module explicitly; not common, but possible).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: Fix the warning messages about casting without __user macro
On Tue, Dec 09, 2014 at 10:56:12PM -0800, Shalin Mehta wrote: > From: Shalin Mehta > > This issue is showed up while compiling with sparse. The iov_base in struct > iovec struct explicitly declares that the assigned value should be user space > pointer with __user macro. Where as here, the __user macro isn't used while > casting. ... and pointers are not user space ones at all. Which is to say, quit messing with casts; it's not struct iovec. Proper fix is to replace it here (and in almost all places throughout drivers/staging/lustre) with struct kvec. And yes, such a patch had been sent. Still not applied, AFAICS... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging:lustre:lnet: Incorrect type in assignment
On Mon, Dec 01, 2014 at 02:53:52PM +0800, sunwxg wrote:
> From: Sun Wang
>
> Incorrect type when assign value to varible iov_base.
Oh, for pity sake... Folks, these struct iovec in there are bloody wrong.
It's not iovec at all. There are exactly two declarations in the whole
drivers/staging/lustre where struct iovec should not be struct kvec. As for
everything else, see below.
commit 82fa028f2b462743aeeb9b973d02733801b2f8e6
Author: Al Viro
Date: Sat Nov 29 12:24:20 2014 -0500
lustre: don't use iovec instead of kvec
Signed-off-by: Al Viro
diff --git a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
index 7e89b3b..99fb52a 100644
--- a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
+++ b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
@@ -752,9 +752,9 @@ int lnet_fail_nid(lnet_nid_t nid, unsigned int threshold);
void lnet_counters_get(lnet_counters_t *counters);
void lnet_counters_reset(void);
-unsigned int lnet_iov_nob(unsigned int niov, struct iovec *iov);
-int lnet_extract_iov(int dst_niov, struct iovec *dst,
-int src_niov, struct iovec *src,
+unsigned int lnet_iov_nob(unsigned int niov, struct kvec *iov);
+int lnet_extract_iov(int dst_niov, struct kvec *dst,
+int src_niov, struct kvec *src,
unsigned int offset, unsigned int len);
unsigned int lnet_kiov_nob(unsigned int niov, lnet_kiov_t *iov);
@@ -762,17 +762,17 @@ int lnet_extract_kiov(int dst_niov, lnet_kiov_t *dst,
int src_niov, lnet_kiov_t *src,
unsigned int offset, unsigned int len);
-void lnet_copy_iov2iov(unsigned int ndiov, struct iovec *diov,
+void lnet_copy_iov2iov(unsigned int ndiov, struct kvec *diov,
unsigned int doffset,
- unsigned int nsiov, struct iovec *siov,
+ unsigned int nsiov, struct kvec *siov,
unsigned int soffset, unsigned int nob);
-void lnet_copy_kiov2iov(unsigned int niov, struct iovec *iov,
+void lnet_copy_kiov2iov(unsigned int niov, struct kvec *iov,
unsigned int iovoffset,
unsigned int nkiov, lnet_kiov_t *kiov,
unsigned int kiovoffset, unsigned int nob);
void lnet_copy_iov2kiov(unsigned int nkiov, lnet_kiov_t *kiov,
unsigned int kiovoffset,
-unsigned int niov, struct iovec *iov,
+unsigned int niov, struct kvec *iov,
unsigned int iovoffset, unsigned int nob);
void lnet_copy_kiov2kiov(unsigned int ndkiov, lnet_kiov_t *dkiov,
unsigned int doffset,
@@ -781,10 +781,10 @@ void lnet_copy_kiov2kiov(unsigned int ndkiov, lnet_kiov_t
*dkiov,
static inline void
lnet_copy_iov2flat(int dlen, void *dest, unsigned int doffset,
- unsigned int nsiov, struct iovec *siov, unsigned int soffset,
+ unsigned int nsiov, struct kvec *siov, unsigned int soffset,
unsigned int nob)
{
- struct iovec diov = {/*.iov_base = */ dest, /*.iov_len = */ dlen};
+ struct kvec diov = {/*.iov_base = */ dest, /*.iov_len = */ dlen};
lnet_copy_iov2iov(1, &diov, doffset,
nsiov, siov, soffset, nob);
@@ -795,17 +795,17 @@ lnet_copy_kiov2flat(int dlen, void *dest, unsigned int
doffset,
unsigned int nsiov, lnet_kiov_t *skiov,
unsigned int soffset, unsigned int nob)
{
- struct iovec diov = {/* .iov_base = */ dest, /* .iov_len = */ dlen};
+ struct kvec diov = {/* .iov_base = */ dest, /* .iov_len = */ dlen};
lnet_copy_kiov2iov(1, &diov, doffset,
nsiov, skiov, soffset, nob);
}
static inline void
-lnet_copy_flat2iov(unsigned int ndiov, struct iovec *diov, unsigned int
doffset,
+lnet_copy_flat2iov(unsigned int ndiov, struct kvec *diov, unsigned int doffset,
int slen, void *src, unsigned int soffset, unsigned int nob)
{
- struct iovec siov = {/*.iov_base = */ src, /*.iov_len = */slen};
+ struct kvec siov = {/*.iov_base = */ src, /*.iov_len = */slen};
lnet_copy_iov2iov(ndiov, diov, doffset,
1, &siov, soffset, nob);
@@ -816,7 +816,7 @@ lnet_copy_flat2kiov(unsigned int ndiov, lnet_kiov_t *dkiov,
unsigned int doffset, int slen, void *src,
unsigned int soffset, unsigned int nob)
{
- struct iovec siov = {/* .iov_base = */ src, /* .iov_len = */ slen};
+ struct kvec siov = {/* .iov_base = */ src, /* .iov_len = */ slen};
lnet_copy_iov2kiov(ndiov, dkiov, doffset,
1, &siov, soffset, nob);
diff --git a/drivers/staging/lustre/include/linux/lnet/lib-types.h
b/drivers/staging/lustre/include/linux/lnet/lib-types.h
Re: [PATCH] staging: lustre: fix pointer declarations
On Tue, Nov 25, 2014 at 09:44:21PM +0100, Zahari Doychev wrote: > This patch fixes pointer declarations from void * to void __user * in order > to remove some sparse warnings. _Are_ those userland addresses, though? Quick grep shows that in the only caller of lnet_copy_iov2flat() we have something called ibmsg passed as the second argument *AND* *RIGHT* *BEFORE* *THAT* *CALL* *WE* *HAVE* ibmsg = tx->tx_msg; ibmsg->ibm_u.immediate.ibim_hdr = *hdr; Go ahead, explain how does that manage to work if ibmsg is a userland pointer. Either you have discovered an exploitable hole (direct store to userland address), or it's not a userland pointer, after all. Al, sick and tired of the "remove some warnings" as the sole rationale for patches, without even an attempt to figure out what those warnings are about. Magic box makes noises, magic box must be appeased... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: socklnd: sparse warning fix
> the same value casted the same way few lines below: > csum = ksocknal_csum(~0, (void *)tx->tx_iov[0].iov_base, > > then it seems like a typo in LASSERT() that is got fixed > > drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c > b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c > index 245c9d7..ebd400d 100644 > --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c > +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c > @@ -390,7 +390,7 @@ ksocknal_lib_csum_tx(ksock_tx_t *tx) > __u32 csum; > void*base; > > - LASSERT(tx->tx_iov[0].iov_base == (void *)&tx->tx_msg); > + LASSERT((void *)tx->tx_iov[0].iov_base == &tx->tx_msg); Turn tx_iov into struct kvec * instead. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3.18-rc2] staging: android: logger: Fix log corruption regression
On Mon, Oct 27, 2014 at 06:51:43PM +, Daniel Thompson wrote:
> Since commit cd678fce4280 ("switch logger to ->write_iter()"), any
> attempt to write to the log results in the log data being written over
> its own metadata, thus rendering the log unreadable.
>
> The problem was first detected when I ran an Android userspace on the
> v3.18-rc1 kernel. However the issue can also be observed with a
> non-Android userspace by using echo/cat to write to/from /dev/log_main .
>
> This patch resolves the problem by using a temporary to track the status
> of not-yet-committed writes to the log buffer.
>
> Signed-off-by: Daniel Thompson
> Cc: Al Viro
Applied
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH resend] staging, lustre: fix a sparse error
On Fri, Oct 10, 2014 at 11:21:16AM +0800, WANG Chao wrote: > I think __user annotation is for no dereferencing in kernel space. In > this case, I think it's fine to override this error by __force. Because > they're pointers with identical target types. Umm... The real question seems to be whether iovec is the right type in the first place. Does ->tx_iov ever contain a vector with _userland_ pointers? If not, it ought to be struct kvec * instead. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
On Wed, Sep 11, 2013 at 05:04:17PM -0700, Joe Perches wrote: > On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote: > > Joe Perches wrote: > > > - seq_printf(m, "%s%d%n", con->name, con->index, &len); > > > + len = seq_printf(m, "%s%d", con->name, con->index); > > > > Isn't len always 0 or -1 ? > > Right. Well you're no fun... > > These uses would seem broken anyway because the > seq_printf isn't itself tested for correctness. > > Hmm. > > Also, there's a large amount of code that appears > to do calculations with pos or len like: > > pos += seq_printf(handle, fmt. ...) ... and most of that code proceeds to ignore pos completely. Note that ->show() is *NOT* supposed to return the number of characters it has/would like to have produced. Just return 0 and be done with that; overflows are dealt with just fine. The large amount, BTW, is below 100 lines, AFAICS, in rather few files. > There are very few that seem to use it correctly > like netfilter. > Suggestions? Just bury the cargo-culting crap. All those += seq_printf() should be simply calling it. The *only* reason to look at the return value is "if we'd already overflown the buffer, I'd rather skipped the costly generation of the rest of the record". In that case seq_printf() returning -1 means "skip it, nothing else will fit and caller will be repeating with bigger buffer anyway". ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
