Re: [PATCH] erofs: Use common kernel logging style
Hi Joe, On Sun, Aug 18, 2019 at 10:47:17PM -0700, Joe Perches wrote: > On Mon, 2019-08-19 at 13:52 +0800, Gao Xiang wrote: > > Hi Joe, > > Hello. > > > On Sun, Aug 18, 2019 at 10:28:41PM -0700, Joe Perches wrote: > > > Rename errln, infoln, and debugln to the typical pr_ uses > > > to the typical kernel styles of pr_ > > > > How about using erofs_err / ... to instead that? > > I've no opinion. > It seems most fs/*/* filesystems actually do use pr_ > sed works well if you want that. Sorry, I mainly refer to ext4, ext2, xfs and f2fs... I didn't notice the other filesystems, you are right. Okay, I have no opinion as well (maybe we could turn back later to introduce sb parameter...) > > > - I can hardly see directly use pr_ for those filesystems in fs/... > > just fyi: > > There was this one existing pr_ use in erofs That is just because the following piece of code was taken from fs/mpage.c, I tend to replace it with iomap in the later version after tail-end packing inline is done. Thanks, Gao Xiang > > drivers/staging/erofs/data.c:366: pr_err("%s, > readahead error at page %lu of nid %llu\n", > drivers/staging/erofs/data.c-367- > __func__, page->index, > drivers/staging/erofs/data.c-368- > EROFS_V(mapping->host)->nid); > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: Use common kernel logging style
On Mon, 2019-08-19 at 13:52 +0800, Gao Xiang wrote: > Hi Joe, Hello. > On Sun, Aug 18, 2019 at 10:28:41PM -0700, Joe Perches wrote: > > Rename errln, infoln, and debugln to the typical pr_ uses > > to the typical kernel styles of pr_ > > How about using erofs_err / ... to instead that? I've no opinion. It seems most fs/*/* filesystems actually do use pr_ sed works well if you want that. > - I can hardly see directly use pr_ for those filesystems in fs/... just fyi: There was this one existing pr_ use in erofs drivers/staging/erofs/data.c:366: pr_err("%s, readahead error at page %lu of nid %llu\n", drivers/staging/erofs/data.c-367- __func__, page->index, drivers/staging/erofs/data.c-368- EROFS_V(mapping->host)->nid); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On 2019年08月19日 13:25, Greg KH wrote: On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote: On 2019年08月16日 14:39, Borislav Petkov wrote: On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: The first three patches are the changes under x86/acrn, which adds the required APIs for the driver and reports the X2APIC caps. The remaining patches add the ACRN driver module, which accepts the ioctl from user-space and then communicate with the low-level ACRN hypervisor by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? Thanks for your reply and the concern. After taking a look at several driver examples(gma500, android), it seems that they are firstly added into drivers/staging/XXX and then moved to drivers/XXX after the driver becomes mature. So we refer to this method to upstream ACRN driver part. Those two examples are probably the worst examples to ever look at :) The code quality of those submissions was horrible, gma500 took a very long time to clean up and there are parts of the android code that are still in staging to this day. If the new driver can also be added by skipping the staging approach, we will refine it and then submit it in normal process. That is the normal process, staging should not be needed at all for any code. It is a fall-back for when the company involved has no idea of how to upstream their code, which should NOT be the case here. Thanks for your explanation. OK. We will submit it in normal process. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] erofs: Use common kernel logging style
Rename errln, infoln, and debugln to the typical pr_ uses to the typical kernel styles of pr_ Miscellanea: o Add newline terminations to the uses o Use "%s: ...", __func__ and not the atypical "%s, ...", __func__ o Trivial grammar changes in output logging o Delete the now unused macros Signed-off-by: Joe Perches --- drivers/staging/erofs/data.c | 6 ++-- drivers/staging/erofs/decompressor.c | 6 ++-- drivers/staging/erofs/dir.c | 8 +++--- drivers/staging/erofs/inode.c| 16 +-- drivers/staging/erofs/internal.h | 8 ++ drivers/staging/erofs/namei.c| 4 +-- drivers/staging/erofs/super.c| 54 +++- drivers/staging/erofs/xattr.c| 4 +-- drivers/staging/erofs/zdata.c| 12 drivers/staging/erofs/zdata.h| 2 +- drivers/staging/erofs/zmap.c | 26 - 11 files changed, 73 insertions(+), 73 deletions(-) diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c index 4cdb743c8b8d..677d95e8fdeb 100644 --- a/drivers/staging/erofs/data.c +++ b/drivers/staging/erofs/data.c @@ -152,8 +152,8 @@ static int erofs_map_blocks_flatmode(struct inode *inode, map->m_flags |= EROFS_MAP_META; } else { - errln("internal error @ nid: %llu (size %llu), m_la 0x%llx", - vi->nid, inode->i_size, map->m_la); + pr_err("internal error @ nid: %llu (size %llu), m_la 0x%llx\n", + vi->nid, inode->i_size, map->m_la); DBG_BUGON(1); err = -EIO; goto err_out; @@ -363,7 +363,7 @@ static int erofs_raw_access_readpages(struct file *filp, /* all the page errors are ignored when readahead */ if (IS_ERR(bio)) { - pr_err("%s, readahead error at page %lu of nid %llu\n", + pr_err("%s: readahead error at page %lu of nid %llu\n", __func__, page->index, EROFS_V(mapping->host)->nid); diff --git a/drivers/staging/erofs/decompressor.c b/drivers/staging/erofs/decompressor.c index 5361a2bbedb6..24d450ce66c1 100644 --- a/drivers/staging/erofs/decompressor.c +++ b/drivers/staging/erofs/decompressor.c @@ -166,9 +166,9 @@ static int lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out) inlen, rq->outputsize, rq->outputsize); if (ret < 0) { - errln("%s, failed to decompress, in[%p, %u, %u] out[%p, %u]", - __func__, src + inputmargin, inlen, inputmargin, - out, rq->outputsize); + pr_err("%s: failed to decompress, in[%p, %u, %u] out[%p, %u]\n", + __func__, src + inputmargin, inlen, inputmargin, + out, rq->outputsize); WARN_ON(1); print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET, 16, 1, src + inputmargin, inlen, true); diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c index 2fbfc4935077..526c7b5dd4db 100644 --- a/drivers/staging/erofs/dir.c +++ b/drivers/staging/erofs/dir.c @@ -29,8 +29,8 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name, memcpy(dbg_namebuf, de_name, de_namelen); dbg_namebuf[de_namelen] = '\0'; - debugln("found dirent %s de_len %u d_type %d", dbg_namebuf, - de_namelen, d_type); + pr_debug("found dirent %s de_len %u d_type %d\n", +dbg_namebuf, de_namelen, d_type); #endif } @@ -104,8 +104,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) if (unlikely(nameoff < sizeof(struct erofs_dirent) || nameoff >= PAGE_SIZE)) { - errln("%s, invalid de[0].nameoff %u", - __func__, nameoff); + pr_err("%s: invalid de[0].nameoff %u\n", + __func__, nameoff); err = -EIO; goto skip_this; diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c index 286729143365..7b91f3baf8d4 100644 --- a/drivers/staging/erofs/inode.c +++ b/drivers/staging/erofs/inode.c @@ -21,8 +21,8 @@ static int read_inode(struct inode *inode, void *data) vi->datamode = __inode_data_mapping(advise); if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) { - errln("unsupported data mapping %u of nid %llu", - vi->datamode, vi->nid); + pr_err("unsupported data mapping %u of nid %llu\n", + vi->datamode, vi->nid); DBG_BUGON(1); return -EIO; } @@ -92,8 +92,8 @@ static int
Re: [RFC PATCH 08/15] drivers/acrn: add VM memory management for ACRN char device
On 2019年08月16日 20:58, Dan Carpenter wrote: On Fri, Aug 16, 2019 at 10:25:49AM +0800, Zhao Yakui wrote: +int hugepage_map_guest(struct acrn_vm *vm, struct vm_memmap *memmap) +{ + struct page *page = NULL, *regions_buf_pg = NULL; + unsigned long len, guest_gpa, vma; + struct vm_memory_region *region_array; + struct set_regions *regions; + int max_size = PAGE_SIZE / sizeof(struct vm_memory_region); + int ret; + + if (!vm || !memmap) + return -EINVAL; + + len = memmap->len; + vma = memmap->vma_base; + guest_gpa = memmap->gpa; + + /* prepare set_memory_regions info */ + regions_buf_pg = alloc_page(GFP_KERNEL); + if (!regions_buf_pg) + return -ENOMEM; + + regions = kzalloc(sizeof(*regions), GFP_KERNEL); + if (!regions) { + __free_page(regions_buf_pg); + return -ENOMEM; It's better to do a goto err_free_regions_buf here. More comments below. + } + regions->mr_num = 0; + regions->vmid = vm->vmid; + regions->regions_gpa = page_to_phys(regions_buf_pg); + region_array = page_to_virt(regions_buf_pg); + + while (len > 0) { + unsigned long vm0_gpa, pagesize; + + ret = get_user_pages_fast(vma, 1, 1, ); + if (unlikely(ret != 1) || (!page)) { + pr_err("failed to pin huge page!\n"); + ret = -ENOMEM; + goto err; goto err is a red flag. It's better if error labels do one specific named thing like: err_regions: kfree(regions); err_free_regions_buf: __free_page(regions_buf_pg); We should unwind in the opposite/mirror order from how things were allocated. Then we can remove the if statements in the error handling. Thanks for the review. Will follow your suggestion to unwind the error handling. In this situation, say the user triggers an -EFAULT in get_user_pages_fast() in the second iteration through the loop. That means that "page" is the non-NULL page from the previous iteration. We have already added it to add_guest_map(). But now we're freeing it without removing it from the map so probably it leads to a use after free. The best way to write the error handling in a loop like this is to clean up the partial iteration that has succeed (nothing here), and then unwind all the successful iterations at the bottom of the function. "goto unwind_loop;" In theory we should cleanup the previous success iteration if it encounters one error in the current iteration. But it will be quite complex to cleanup up the previous iteration. call the set_memory_regions for MR_DEL op. call the remove_guest_map for the added hash item call the put_page for returned page in get_user_pages_fast. In fact as this driver is mainly used for embedded IOT usage, it doesn't handle the complex cleanup when such error is encountered. Instead the clean up is handled in free_guest_vm. + } + + vm0_gpa = page_to_phys(page); + pagesize = PAGE_SIZE << compound_order(page); + + ret = add_guest_map(vm, vm0_gpa, guest_gpa, pagesize); + if (ret < 0) { + pr_err("failed to add memseg for huge page!\n"); + goto err; So then here, it would be: pr_err("failed to add memseg for huge page!\n"); put_page(page); goto unwind_loop; regards, dan carpenter + } + + /* fill each memory region into region_array */ + region_array[regions->mr_num].type = MR_ADD; + region_array[regions->mr_num].gpa = guest_gpa; + region_array[regions->mr_num].vm0_gpa = vm0_gpa; + region_array[regions->mr_num].size = pagesize; + region_array[regions->mr_num].prot = + (MEM_TYPE_WB & MEM_TYPE_MASK) | + (memmap->prot & MEM_ACCESS_RIGHT_MASK); + regions->mr_num++; + if (regions->mr_num == max_size) { + pr_debug("region buffer full, set & renew regions!\n"); + ret = set_memory_regions(regions); + if (ret < 0) { + pr_err("failed to set regions,ret=%d!\n", ret); + goto err; + } + regions->mr_num = 0; + } + + len -= pagesize; + vma += pagesize; + guest_gpa += pagesize; + } + + ret = set_memory_regions(regions); + if (ret < 0) { + pr_err("failed to set regions, ret=%d!\n", ret); + goto err; + } + + __free_page(regions_buf_pg); + kfree(regions); + + return 0; +err: + if (regions_buf_pg) +
Re: [PATCH] erofs: Use common kernel logging style
Hi Joe, On Sun, Aug 18, 2019 at 10:28:41PM -0700, Joe Perches wrote: > Rename errln, infoln, and debugln to the typical pr_ uses > to the typical kernel styles of pr_ How about using erofs_err / ... to instead that? - I can hardly see directly use pr_ for those filesystems in fs/... - maybe we will introduce super_block to print more information about the specific filesystem... > > Miscellanea: > > o Add newline terminations to the uses > o Use "%s: ...", __func__ and not the atypical "%s, ...", __func__ Agreed. Thanks, Gao Xiang > o Trivial grammar changes in output logging > o Delete the now unused macros > > Signed-off-by: Joe Perches > --- > drivers/staging/erofs/data.c | 6 ++-- > drivers/staging/erofs/decompressor.c | 6 ++-- > drivers/staging/erofs/dir.c | 8 +++--- > drivers/staging/erofs/inode.c| 16 +-- > drivers/staging/erofs/internal.h | 8 ++ > drivers/staging/erofs/namei.c| 4 +-- > drivers/staging/erofs/super.c| 54 > +++- > drivers/staging/erofs/xattr.c| 4 +-- > drivers/staging/erofs/zdata.c| 12 > drivers/staging/erofs/zdata.h| 2 +- > drivers/staging/erofs/zmap.c | 26 - > 11 files changed, 73 insertions(+), 73 deletions(-) > > diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c > index 4cdb743c8b8d..677d95e8fdeb 100644 > --- a/drivers/staging/erofs/data.c > +++ b/drivers/staging/erofs/data.c > @@ -152,8 +152,8 @@ static int erofs_map_blocks_flatmode(struct inode *inode, > > map->m_flags |= EROFS_MAP_META; > } else { > - errln("internal error @ nid: %llu (size %llu), m_la 0x%llx", > - vi->nid, inode->i_size, map->m_la); > + pr_err("internal error @ nid: %llu (size %llu), m_la 0x%llx\n", > +vi->nid, inode->i_size, map->m_la); > DBG_BUGON(1); > err = -EIO; > goto err_out; > @@ -363,7 +363,7 @@ static int erofs_raw_access_readpages(struct file *filp, > > /* all the page errors are ignored when readahead */ > if (IS_ERR(bio)) { > - pr_err("%s, readahead error at page %lu of nid > %llu\n", > + pr_err("%s: readahead error at page %lu of nid > %llu\n", > __func__, page->index, > EROFS_V(mapping->host)->nid); > > diff --git a/drivers/staging/erofs/decompressor.c > b/drivers/staging/erofs/decompressor.c > index 5361a2bbedb6..24d450ce66c1 100644 > --- a/drivers/staging/erofs/decompressor.c > +++ b/drivers/staging/erofs/decompressor.c > @@ -166,9 +166,9 @@ static int lz4_decompress(struct z_erofs_decompress_req > *rq, u8 *out) > inlen, rq->outputsize, > rq->outputsize); > if (ret < 0) { > - errln("%s, failed to decompress, in[%p, %u, %u] out[%p, %u]", > - __func__, src + inputmargin, inlen, inputmargin, > - out, rq->outputsize); > + pr_err("%s: failed to decompress, in[%p, %u, %u] out[%p, %u]\n", > +__func__, src + inputmargin, inlen, inputmargin, > +out, rq->outputsize); > WARN_ON(1); > print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET, > 16, 1, src + inputmargin, inlen, true); > diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c > index 2fbfc4935077..526c7b5dd4db 100644 > --- a/drivers/staging/erofs/dir.c > +++ b/drivers/staging/erofs/dir.c > @@ -29,8 +29,8 @@ static void debug_one_dentry(unsigned char d_type, const > char *de_name, > memcpy(dbg_namebuf, de_name, de_namelen); > dbg_namebuf[de_namelen] = '\0'; > > - debugln("found dirent %s de_len %u d_type %d", dbg_namebuf, > - de_namelen, d_type); > + pr_debug("found dirent %s de_len %u d_type %d\n", > + dbg_namebuf, de_namelen, d_type); > #endif > } > > @@ -104,8 +104,8 @@ static int erofs_readdir(struct file *f, struct > dir_context *ctx) > > if (unlikely(nameoff < sizeof(struct erofs_dirent) || >nameoff >= PAGE_SIZE)) { > - errln("%s, invalid de[0].nameoff %u", > - __func__, nameoff); > + pr_err("%s: invalid de[0].nameoff %u\n", > +__func__, nameoff); > > err = -EIO; > goto skip_this; > diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c > index 286729143365..7b91f3baf8d4 100644 > --- a/drivers/staging/erofs/inode.c > +++ b/drivers/staging/erofs/inode.c > @@ -21,8 +21,8 @@ static int read_inode(struct inode *inode, void *data) >
Re: [RFC PATCH 04/15] drivers/acrn: add the basic framework of acrn char device driver
On Mon, Aug 19, 2019 at 12:02:33PM +0800, Zhao, Yakui wrote: > > > On 2019年08月16日 15:05, Greg KH wrote: > > On Fri, Aug 16, 2019 at 10:25:45AM +0800, Zhao Yakui wrote: > > > ACRN hypervisor service module is the important middle layer that allows > > > the Linux kernel to communicate with the ACRN hypervisor. It includes > > > the management of virtualized CPU/memory/device/interrupt for other ACRN > > > guest. The user-space applications can use the provided ACRN ioctls to > > > interact with ACRN hypervisor through different hypercalls. > > > > > > Add one basic framework firstly and the following patches will > > > add the corresponding implementations, which includes the management of > > > virtualized CPU/memory/interrupt and the emulation of MMIO/IO/PCI access. > > > The device file of /dev/acrn_hsm can be accessed in user-space to > > > communicate with ACRN module. > > > > > > Co-developed-by: Jason Chen CJ > > > Signed-off-by: Jason Chen CJ > > > Co-developed-by: Jack Ren > > > Signed-off-by: Jack Ren > > > Co-developed-by: Mingqiang Chi > > > Signed-off-by: Mingqiang Chi > > > Co-developed-by: Liu Shuo > > > Signed-off-by: Liu Shuo > > > Signed-off-by: Zhao Yakui > > > --- > > > drivers/staging/Kconfig | 2 + > > > > Also, your subject line for all of these patches are wrong, it is not > > drivers/acrn :( > > Thanks for the pointing out it. > > It will be fixed. > > > > > And you forgot to cc: the staging maintainer :( > > Do you mean that the maintainer of staging subsystem is also added in the > patch commit log? Did you not run scripts/get_maintainer.pl on your patches to determine who to send patches to? Always do that. > > As I have said with NUMEROUS Intel patches in the past, I now refuse to > > take patches from you all WITHOUT having it signed-off-by someone from > > the Intel "OTC" group (or whatever the Intel Linux group is called these > > days). They are a resource you can not ignore, and if you do, you just > > end up making the rest of the kernel community grumpy by having us do > > their work for them :( > > > > Please work with them. > > OK. I will work with some peoples in OTC group to prepare the better ACRN > driver. Thank you. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Mon, Aug 19, 2019 at 10:39:32AM +0800, Zhao, Yakui wrote: > > > On 2019年08月16日 15:03, Greg KH wrote: > > On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote: > > > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: > > > > The first three patches are the changes under x86/acrn, which adds the > > > > required APIs for the driver and reports the X2APIC caps. > > > > The remaining patches add the ACRN driver module, which accepts the > > > > ioctl > > > > from user-space and then communicate with the low-level ACRN hypervisor > > > > by using hypercall. > > > > > > I have a problem with that: you're adding interfaces to arch/x86/ and > > > its users go into staging. Why? Why not directly put the driver where > > > it belongs, clean it up properly and submit it like everything else is > > > submitted? > > > > > > I don't want to have stuff in arch/x86/ which is used solely by code in > > > staging and the latter is lingering there indefinitely because no one is > > > cleaning it up... > > > > I agree, stuff in drivers/staging/ must be self-contained, with no > > changes outside of the code's subdirectory needed in order for it to > > work. That way it is trivial for us to delete it when it never gets > > cleaned up :) > > Thanks for pointing out the rule of drivers/staging. > The acrn staging driver is one self-contained driver. But it has some > dependency on arch/x86/acrn and need to call the APIs in arch/x86/acrn. Then it should not be in drivers/staging/ Please work to get this accepted "normally". thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote: > > > On 2019年08月16日 14:39, Borislav Petkov wrote: > > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: > > > The first three patches are the changes under x86/acrn, which adds the > > > required APIs for the driver and reports the X2APIC caps. > > > The remaining patches add the ACRN driver module, which accepts the ioctl > > > from user-space and then communicate with the low-level ACRN hypervisor > > > by using hypercall. > > > > I have a problem with that: you're adding interfaces to arch/x86/ and > > its users go into staging. Why? Why not directly put the driver where > > it belongs, clean it up properly and submit it like everything else is > > submitted? > > Thanks for your reply and the concern. > > After taking a look at several driver examples(gma500, android), it seems > that they are firstly added into drivers/staging/XXX and then moved to > drivers/XXX after the driver becomes mature. > So we refer to this method to upstream ACRN driver part. Those two examples are probably the worst examples to ever look at :) The code quality of those submissions was horrible, gma500 took a very long time to clean up and there are parts of the android code that are still in staging to this day. > If the new driver can also be added by skipping the staging approach, > we will refine it and then submit it in normal process. That is the normal process, staging should not be needed at all for any code. It is a fall-back for when the company involved has no idea of how to upstream their code, which should NOT be the case here. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 10/15] drivers/acrn: add interrupt injection support
On 2019年08月16日 21:12, Dan Carpenter wrote: On Fri, Aug 16, 2019 at 10:25:51AM +0800, Zhao Yakui wrote: + case IC_VM_INTR_MONITOR: { + struct page *page; + + ret = get_user_pages_fast(ioctl_param, 1, 1, ); + if (unlikely(ret != 1) || !page) { Not required. Do you mean that it is enough to check the condition of "ret != 1"? OK. It will be removed. + pr_err("acrn-dev: failed to pin intr hdr buffer!\n"); + return -ENOMEM; + } + + ret = hcall_vm_intr_monitor(vm->vmid, page_to_phys(page)); + if (ret < 0) { + pr_err("acrn-dev: monitor intr data err=%ld\n", ret); + return -EFAULT; + } + break; + } + regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 11/15] drivers/acrn: add the support of handling emulated ioreq
On 2019年08月16日 21:39, Dan Carpenter wrote: On Fri, Aug 16, 2019 at 10:25:52AM +0800, Zhao Yakui wrote: +int acrn_ioreq_create_client(unsigned short vmid, +ioreq_handler_t handler, +void *client_priv, +char *name) +{ + struct acrn_vm *vm; + struct ioreq_client *client; + int client_id; + + might_sleep(); + + vm = find_get_vm(vmid); + if (unlikely(!vm || !vm->req_buf)) { + pr_err("acrn-ioreq: failed to find vm from vmid %d\n", vmid); + put_vm(vm); + return -EINVAL; + } + + client_id = alloc_client(); + if (unlikely(client_id < 0)) { + pr_err("acrn-ioreq: vm[%d] failed to alloc ioreq client\n", + vmid); + put_vm(vm); + return -EINVAL; + } + + client = acrn_ioreq_get_client(client_id); + if (unlikely(!client)) { + pr_err("failed to get the client.\n"); + put_vm(vm); + return -EINVAL; Do we need to clean up the alloc_client() allocation? Thanks for the review. The function of acrn_iocreq_get_client is used to return the client for the given client_id. (The ref_count of client is also added). If it is NULL, it indicates that it is already released in another place. In the function of acrn_ioreq_create_client, we don't need to clean up the alloc_client as it always exists in course of creating_client. regards, dan carpenter + } + + if (handler) { + client->handler = handler; + client->acrn_create_kthread = true; + } + + client->ref_vm = vm; + client->vmid = vmid; + client->client_priv = client_priv; + if (name) + strncpy(client->name, name, sizeof(client->name) - 1); + rwlock_init(>range_lock); + INIT_LIST_HEAD(>range_list); + init_waitqueue_head(>wq); + + /* When it is added to ioreq_client_list, the refcnt is increased */ + spin_lock_bh(>ioreq_client_lock); + list_add(>list, >ioreq_client_list); + spin_unlock_bh(>ioreq_client_lock); + + pr_info("acrn-ioreq: created ioreq client %d\n", client_id); + + return client_id; +} ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: hantro: Remove call to memset after dma_alloc_coherent
Hi Hans, On Thu, Jul 25, 2019 at 8:50 PM Boris Brezillon wrote: > > On Thu, 25 Jul 2019 08:36:02 +0530 > Hariprasad Kelam wrote: > > > fix below issue reported by coccicheck > > /drivers/staging/media/hantro/hantro_vp8.c:149:16-34: WARNING: > > dma_alloc_coherent use in aux_buf -> cpu already zeroes out memory, so > > memset is not needed > > > > Signed-off-by: Hariprasad Kelam > > Reviewed-by: Boris Brezillon > > > --- > > drivers/staging/media/hantro/hantro_vp8.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_vp8.c > > b/drivers/staging/media/hantro/hantro_vp8.c > > index 66c4533..363ddda 100644 > > --- a/drivers/staging/media/hantro/hantro_vp8.c > > +++ b/drivers/staging/media/hantro/hantro_vp8.c > > @@ -151,8 +151,6 @@ int hantro_vp8_dec_init(struct hantro_ctx *ctx) > > if (!aux_buf->cpu) > > return -ENOMEM; > > > > - memset(aux_buf->cpu, 0, aux_buf->size); > > - > > /* > >* Allocate probability table buffer, > >* total 1208 bytes, 4K page is far enough. > Is this something you will pick to your tree? Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: hantro: Remove call to memset after dma_alloc_coherent
On Mon, Aug 19, 2019 at 1:17 PM Tomasz Figa wrote: > > Hi Hans, > > On Thu, Jul 25, 2019 at 8:50 PM Boris Brezillon > wrote: > > > > On Thu, 25 Jul 2019 08:36:02 +0530 > > Hariprasad Kelam wrote: > > > > > fix below issue reported by coccicheck > > > /drivers/staging/media/hantro/hantro_vp8.c:149:16-34: WARNING: > > > dma_alloc_coherent use in aux_buf -> cpu already zeroes out memory, so > > > memset is not needed > > > > > > Signed-off-by: Hariprasad Kelam > > > > Reviewed-by: Boris Brezillon > > > > > --- > > > drivers/staging/media/hantro/hantro_vp8.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/drivers/staging/media/hantro/hantro_vp8.c > > > b/drivers/staging/media/hantro/hantro_vp8.c > > > index 66c4533..363ddda 100644 > > > --- a/drivers/staging/media/hantro/hantro_vp8.c > > > +++ b/drivers/staging/media/hantro/hantro_vp8.c > > > @@ -151,8 +151,6 @@ int hantro_vp8_dec_init(struct hantro_ctx *ctx) > > > if (!aux_buf->cpu) > > > return -ENOMEM; > > > > > > - memset(aux_buf->cpu, 0, aux_buf->size); > > > - > > > /* > > >* Allocate probability table buffer, > > >* total 1208 bytes, 4K page is far enough. > > > > Is this something you will pick to your tree? Ah, sorry, this is already applied. Not sure why searching for it the first time didn't show anything. I guess I need to start repeating my searches by default. Sorry for the noise. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 04/15] drivers/acrn: add the basic framework of acrn char device driver
On 2019年08月16日 15:05, Greg KH wrote: On Fri, Aug 16, 2019 at 10:25:45AM +0800, Zhao Yakui wrote: ACRN hypervisor service module is the important middle layer that allows the Linux kernel to communicate with the ACRN hypervisor. It includes the management of virtualized CPU/memory/device/interrupt for other ACRN guest. The user-space applications can use the provided ACRN ioctls to interact with ACRN hypervisor through different hypercalls. Add one basic framework firstly and the following patches will add the corresponding implementations, which includes the management of virtualized CPU/memory/interrupt and the emulation of MMIO/IO/PCI access. The device file of /dev/acrn_hsm can be accessed in user-space to communicate with ACRN module. Co-developed-by: Jason Chen CJ Signed-off-by: Jason Chen CJ Co-developed-by: Jack Ren Signed-off-by: Jack Ren Co-developed-by: Mingqiang Chi Signed-off-by: Mingqiang Chi Co-developed-by: Liu Shuo Signed-off-by: Liu Shuo Signed-off-by: Zhao Yakui --- drivers/staging/Kconfig | 2 + Also, your subject line for all of these patches are wrong, it is not drivers/acrn :( Thanks for the pointing out it. It will be fixed. And you forgot to cc: the staging maintainer :( Do you mean that the maintainer of staging subsystem is also added in the patch commit log? As I have said with NUMEROUS Intel patches in the past, I now refuse to take patches from you all WITHOUT having it signed-off-by someone from the Intel "OTC" group (or whatever the Intel Linux group is called these days). They are a resource you can not ignore, and if you do, you just end up making the rest of the kernel community grumpy by having us do their work for them :( Please work with them. OK. I will work with some peoples in OTC group to prepare the better ACRN driver. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On 2019年08月16日 15:03, Greg KH wrote: On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote: On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: The first three patches are the changes under x86/acrn, which adds the required APIs for the driver and reports the X2APIC caps. The remaining patches add the ACRN driver module, which accepts the ioctl from user-space and then communicate with the low-level ACRN hypervisor by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? I don't want to have stuff in arch/x86/ which is used solely by code in staging and the latter is lingering there indefinitely because no one is cleaning it up... I agree, stuff in drivers/staging/ must be self-contained, with no changes outside of the code's subdirectory needed in order for it to work. That way it is trivial for us to delete it when it never gets cleaned up :) Thanks for pointing out the rule of drivers/staging. The acrn staging driver is one self-contained driver. But it has some dependency on arch/x86/acrn and need to call the APIs in arch/x86/acrn. If there is no driver, the API without user had better not be added. If API is not added, the driver can't be compiled correctly. The ACRN driver is one new driver. Maybe it will have some bugs and not be mature. So we want to add the driver as the staging. What is the better approach to handle such scenario? You never say _why_ this should go into drivers/staging/, nor do you have a TODO file like all other staging code that explains exactly what needs to be done to get it out of there. Ok. The TODO file will be added in next version. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 00/15] acrn: add the ACRN driver module
On 2019年08月16日 14:39, Borislav Petkov wrote: On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote: The first three patches are the changes under x86/acrn, which adds the required APIs for the driver and reports the X2APIC caps. The remaining patches add the ACRN driver module, which accepts the ioctl from user-space and then communicate with the low-level ACRN hypervisor by using hypercall. I have a problem with that: you're adding interfaces to arch/x86/ and its users go into staging. Why? Why not directly put the driver where it belongs, clean it up properly and submit it like everything else is submitted? Thanks for your reply and the concern. After taking a look at several driver examples(gma500, android), it seems that they are firstly added into drivers/staging/XXX and then moved to drivers/XXX after the driver becomes mature. So we refer to this method to upstream ACRN driver part. If the new driver can also be added by skipping the staging approach, we will refine it and then submit it in normal process. I don't want to have stuff in arch/x86/ which is used solely by code in staging and the latter is lingering there indefinitely because no one is cleaning it up... The ACRN driver will be the only user of the added APIs in x86/acrn. Without the APIs in x86/acrn, the driver can't add the driver-specifc upcall notification ISR or call the hypercall. Not sure whether it can be sent in two patch sets? The first is to add the required APIs for ACRN driver. The second is to add the ACRN driver Thanks Yakui ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [BUG] removing and reinserting imx-media causes kernel to explode
On 8/14/19 2:25 AM, Russell King - ARM Linux admin wrote: I just did this: rmmod imx-media modprobe imx-media and was greeted by the below kernel messages. Yes this needs fixing, the CSI needs to check first that it is already registered before going through the ->registered() steps. Posting a patch. I don't think this has been the first issue I found with the iMX media stuff involving a module unload/reload cycle - may I suggest that this is added to the testing regime for this code? Thanks. I do test module unload/reload cycles, but v4l2-async does not support re-registering subdevices unless the subdevice is basically completely removed and re-probed, so there won't be a working media device if only imx-media is reloaded. So I have always unloaded/reloaded all modules for every entity in the graph, i.e.: rmmod imx6_media imx6_media_csi imx6_mipi_csi2 ov5640 video_mux imx_media_common (replace ov5640 with your imx219 sensor). But I'll make sure to test single module unload/reload cycles in the future. But note after applying the patch mentioned above to CSI ->registered() callback, there are list corruption backtraces, see [1]. The root cause is that both media_device_register_entity() and media_entity_pads_init() add the same graph objects for the entity's pads, so duplicate pad objects are added to the media device pads list. Removing the pad object creation in media_device_register_entity() fixes the list corruption. Sending a patch for that also. This is a problem for any entity that sets its ->num_pads to a non-zero value before media_device_register_entity() is called. For example, the following will produce the same list corruption backtrace: rmmod video-mux modprobe video-mux rmmod video-mux Steve [1] rmmod imx6-media modprobe imx6-media rmmod imx6-media [ 249.387953] WARNING: CPU: 2 PID: 843 at lib/list_debug.c:53 __list_del_entry_valid+0xa0/0xdc [ 249.396442] list_del corruption. prev->next should be e8fb0510, but was e93b5914 [ 249.404076] Modules linked in: imx6_media_csi(C) imx6_media(C-) imx6_mipi_csi2(C) bnep dw_hdmi_ahb_audio dw_hdmi_cec ov5640 mux_mmio video_mux mux_core dw_hdmi_imx dw_hdmi coda_vpu cec imx_vdoa videobuf2_vmalloc imx_media_common(C) v4l2_fwnode imx_ldb imxdrm imx_ipu_v3 [last unloaded: imx6_media_csi] [ 249.430956] CPU: 2 PID: 843 Comm: rmmod Tainted: G C 5.3.0-rc4-01115-g62119fd20fda #5 [ 249.440115] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 249.446689] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 249.454462] [] (show_stack) from [] (dump_stack+0xd8/0x110) [ 249.461804] [] (dump_stack) from [] (__warn+0xe0/0x10c) [ 249.468789] [] (__warn) from [] (warn_slowpath_fmt+0x44/0x6c) [ 249.476301] [] (warn_slowpath_fmt) from [] (__list_del_entry_valid+0xa0/0xdc) [ 249.485207] [] (__list_del_entry_valid) from [] (media_gobj_destroy.part.4+0x30/0x64) [ 249.494802] [] (media_gobj_destroy.part.4) from [] (__media_device_unregister_entity+0xa8/0xcc) [ 249.505259] [] (__media_device_unregister_entity) from [] (media_device_unregister_entity+0x2c/0x38) [ 249.516157] [] (media_device_unregister_entity) from [] (v4l2_device_unregister_subdev+0x90/0xb4) [ 249.526793] [] (v4l2_device_unregister_subdev) from [] (v4l2_async_cleanup+0x10/0x3c) [ 249.536382] [] (v4l2_async_cleanup) from [] (v4l2_async_notifier_unbind_all_subdevs+0x9c/0x10c) [ 249.546840] [] (v4l2_async_notifier_unbind_all_subdevs) from [] (v4l2_async_notifier_unbind_all_subdevs+0x6c/0x10c) [ 249.559035] [] (v4l2_async_notifier_unbind_all_subdevs) from [] (__v4l2_async_notifier_unregister.part.4+0xc/0x44) [ 249.571140] [] (__v4l2_async_notifier_unregister.part.4) from [] (v4l2_async_notifier_unregister+0x30/0x50) [ 249.582665] [] (v4l2_async_notifier_unregister) from [] (imx_media_remove+0x20/0x54 [imx6_media]) [ 249.593389] [] (imx_media_remove [imx6_media]) from [] (platform_drv_remove+0x20/0x40) [ 249.603068] [] (platform_drv_remove) from [] (device_release_driver_internal+0xdc/0x1ac) [ 249.612917] [] (device_release_driver_internal) from [] (driver_detach+0x44/0x80) [ 249.622164] [] (driver_detach) from [] (bus_remove_driver+0x5c/0xd8) [ 249.630287] [] (bus_remove_driver) from [] (sys_delete_module+0x17c/0x20c) [ 249.638926] [] (sys_delete_module) from [] (ret_fast_syscall+0x0/0x28) [ 249.647205] Exception stack(0xe90d5fa8 to 0xe90d5ff0) [ 249.652278] 5fa0: beed5d6c 0003 01401134 0800 4f13b6f4 2002 [ 249.660475] 5fc0: beed5d6c 0003 beed5b80 0081 beed5e78 0001 014010f8 [ 249.668669] 5fe0: 0003b2c4 beed5b4c 0001f248 4f1012dc [ 249.673859] irq event stamp: 4113 [ 249.677267] hardirqs last enabled at (4131): [] console_unlock+0x408/0x5f8 [ 249.685125] hardirqs last disabled at (4138): [] console_unlock+0x88/0x5f8 [ 249.692970] softirqs last enabled at (4154): [] __do_softirq+0x360/0x524 [ 249.700735] softirqs last disabled at
Re: [PATCH] erofs: move erofs out of staging
Hi all, On Mon, Aug 19, 2019 at 02:16:55AM +0800, Gao Xiang wrote: > Hi Hch, > > On Sun, Aug 18, 2019 at 10:47:02AM -0700, Christoph Hellwig wrote: > > On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote: > > > Not sure what you're even disagreeing with, as I *do* expect new > > > filesystems to > > > be held to a high standard, and to be written with the assumption that the > > > on-disk data may be corrupted or malicious. We just can't expect the bar > > > to be > > > so high (e.g. no bugs) that it's never been attained by *any* filesystem > > > even > > > after years/decades of active development. If the developers were > > > careful, the > > > code generally looks robust, and they are willing to address such bugs as > > > they > > > are found, realistically that's as good as we can expect to get... > > > > Well, the impression I got from Richards quick look and the reply to it is > > that there is very little attempt to validate the ondisk data structure > > and there is absolutely no priority to do so. Which is very different > > from there is a bug or two here and there. > > As my second reply to Richard, I didn't fuzz all the on-disk fields for EROFS. > and as my reply to Richard / Greg, current EROFS is used on the top of > dm-verity. > > I cannot say how well EROFS will be performed on malformed images (and you can > also find the bug richard pointed out is a miswritten break->continue by > myself). > > I posted the upstream EROFS post on July 4, 2019 and a month and a half later, > no one can tell me (yes, thanks for kind people reply me about their > suggestion) > what we should do next (you can see these emails, I sent many times) to meet > the minimal upstream requirements and rare people can even dip into my code. > > That is all I want to say. I will work on autofuzz these days, and I want to > know how to meet your requirements on this (you can tell us your standard, > how well should we do). > > OK, you don't reply to my post once, I have no idea how to get your first > reply. I have made a simple fuzzer to inject messy in inode metadata, dir data, compressed indexes and super block, https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=experimental-fuzzer I am testing with some given dirs and the following script. Does it look reasonable? # !/bin/bash mkdir -p mntdir for ((i=0; i<1000; ++i)); do mkfs/mkfs.erofs -F$i testdir_fsl.fuzz.img testdir_fsl > /dev/null 2>&1 umount mntdir mount -t erofs -o loop testdir_fsl.fuzz.img mntdir for j in `find mntdir -type f`; do md5sum $j > /dev/null done done Thanks, Gao Xiang > > Thanks, > Gao Xiang > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: iio: accel: adis16240: Improve readability on write_raw function
On Wed, 14 Aug 2019 06:56:18 + "Ardelean, Alexandru" wrote: > On Tue, 2019-08-13 at 16:31 -0300, Rodrigo Ribeiro wrote: > > [External] > > > > Replace shift and minus operation by GENMASK macro and remove the local > > variables used to store intermediate data. > > > > Reviewed-by: Alexandru Ardelean Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > > > Signed-off-by: Rodrigo Ribeiro Carvalho > > --- > > v2: > >- Leave switch statement instead of replace by if statement > > drivers/staging/iio/accel/adis16240.c | 5 + > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/staging/iio/accel/adis16240.c > > b/drivers/staging/iio/accel/adis16240.c > > index 62f4b3b1b457..82099db4bf0c 100644 > > --- a/drivers/staging/iio/accel/adis16240.c > > +++ b/drivers/staging/iio/accel/adis16240.c > > @@ -309,15 +309,12 @@ static int adis16240_write_raw(struct iio_dev > > *indio_dev, > >long mask) > > { > > struct adis *st = iio_priv(indio_dev); > > - int bits = 10; > > - s16 val16; > > u8 addr; > > > > switch (mask) { > > case IIO_CHAN_INFO_CALIBBIAS: > > - val16 = val & ((1 << bits) - 1); > > addr = adis16240_addresses[chan->scan_index][0]; > > - return adis_write_reg_16(st, addr, val16); > > + return adis_write_reg_16(st, addr, val & GENMASK(9, 0)); > > } > > return -EINVAL; > > } ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rts5208: remove redundant assignment to retval
From: Colin Ian King Variable retval is initialized to a value that is never read and it is re-assigned later. The initialization is redundant and can be removed. Addresses-Coverity: ("Unused value") Signed-off-by: Colin Ian King --- drivers/staging/rts5208/ms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c index 1128eec3bd08..e853fa9cc950 100644 --- a/drivers/staging/rts5208/ms.c +++ b/drivers/staging/rts5208/ms.c @@ -3842,7 +3842,7 @@ int mg_set_leaf_id(struct scsi_cmnd *srb, struct rtsx_chip *chip) int mg_get_local_EKB(struct scsi_cmnd *srb, struct rtsx_chip *chip) { - int retval = STATUS_FAIL; + int retval; int bufflen; unsigned int lun = SCSI_LUN(srb); u8 *buf = NULL; -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
Hi Richard, On Sun, Aug 18, 2019 at 08:00:28PM +0200, Richard Weinberger wrote: > - Ursprüngliche Mail - > > Von: "tytso" > > An: "richard" > > CC: "Greg Kroah-Hartman" , "Gao Xiang" > > , "Jan Kara" , "Chao > > Yu" , "Dave Chinner" , "David > > Sterba" , "Miao Xie" > > , "devel" , "Stephen > > Rothwell" , "Darrick" > > , "Christoph Hellwig" , "Amir > > Goldstein" , > > "linux-erofs" , "Al Viro" > > , "Jaegeuk Kim" , > > "linux-kernel" , "Li Guifu" > > , "Fang Wei" , > > "Pavel Machek" , "linux-fsdevel" > > , "Andrew Morton" > > , "torvalds" > > Gesendet: Sonntag, 18. August 2019 19:46:21 > > Betreff: Re: [PATCH] erofs: move erofs out of staging > > > On Sun, Aug 18, 2019 at 07:06:40PM +0200, Richard Weinberger wrote: > >> > So holding a file system like EROFS to a higher standard than say, > >> > ext4, xfs, or btrfs hardly seems fair. > >> > >> Nobody claimed that. > > > > Pointing out that erofs has issues in this area when Gao Xiang is > > asking if erofs can be moved out of staging and join the "official > > clubhouse" of file systems could certainly be reasonable interpreted > > as such. Reporting such vulnerablities are a good thing, and > > hopefully all file system maintainers will welcome them. Doing them > > on a e-mail thread about promoting out of erofs is certainly going to > > lead to inferences of a double standard. > > Well, this was not at all my intention. > erofs raised my attention and instead of wasting a new thread > I answered here and reported what I found while looking at it. > That's all. Thank you very much, EROFS finally has some real concern after a quite long time. I will do that but I really want to upstream for 5.4LTS and hope to get your further report. Thanks, Gao Xiang > > Thanks, > //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: kpc2000: kpc2000_i2c: Fix different address spaces warnings
This patch fixes the following sparse warnings: kpc2000_i2c.c:137: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:137:expected void const volatile [noderef] *addr kpc2000_i2c.c:137:got void * kpc2000_i2c.c:146: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:146:expected void volatile [noderef] *addr kpc2000_i2c.c:146:got void * kpc2000_i2c.c:147: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:147:expected void const volatile [noderef] *addr kpc2000_i2c.c:147:got void * kpc2000_i2c.c:166: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:166:expected void const volatile [noderef] *addr kpc2000_i2c.c:166:got void * kpc2000_i2c.c:166: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:166:expected void volatile [noderef] *addr kpc2000_i2c.c:166:got void * kpc2000_i2c.c:168: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:168:expected void const volatile [noderef] *addr kpc2000_i2c.c:168:got void * kpc2000_i2c.c:168: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:168:expected void volatile [noderef] *addr kpc2000_i2c.c:168:got void * kpc2000_i2c.c:171: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:171:expected void const volatile [noderef] *addr kpc2000_i2c.c:171:got void * kpc2000_i2c.c:174: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:174:expected void volatile [noderef] *addr kpc2000_i2c.c:174:got void * kpc2000_i2c.c:193: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:193:expected void volatile [noderef] *addr kpc2000_i2c.c:193:got void * kpc2000_i2c.c:194: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:194:expected void const volatile [noderef] *addr kpc2000_i2c.c:194:got void * kpc2000_i2c.c:214: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:214:expected void volatile [noderef] *addr kpc2000_i2c.c:214:got void * kpc2000_i2c.c:219: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:219:expected void const volatile [noderef] *addr kpc2000_i2c.c:219:got void * kpc2000_i2c.c:226: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:226:expected void volatile [noderef] *addr kpc2000_i2c.c:226:got void * kpc2000_i2c.c:238: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:238:expected void const volatile [noderef] *addr kpc2000_i2c.c:238:got void * kpc2000_i2c.c:244: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:244:expected void volatile [noderef] *addr kpc2000_i2c.c:244:got void * kpc2000_i2c.c:252: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:252:expected void const volatile [noderef] *addr kpc2000_i2c.c:252:got void * kpc2000_i2c.c:257: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:257:expected void volatile [noderef] *addr kpc2000_i2c.c:257:got void * kpc2000_i2c.c:259: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:259:expected void volatile [noderef] *addr kpc2000_i2c.c:259:got void * kpc2000_i2c.c:267: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:267:expected void const volatile [noderef] *addr kpc2000_i2c.c:267:got void * kpc2000_i2c.c:273: warning: incorrect type in argument 1 (different address spaces) kpc2000_i2c.c:273:expected void const volatile [noderef] *addr kpc2000_i2c.c:273:got void * kpc2000_i2c.c:293: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:293:expected void volatile [noderef] *addr kpc2000_i2c.c:293:got void * kpc2000_i2c.c:294: warning: incorrect type in argument 2 (different address spaces) kpc2000_i2c.c:294:expected void volatile [noderef] *addr
Re: [PATCH] erofs: move erofs out of staging
Hi Hch, On Sun, Aug 18, 2019 at 10:47:02AM -0700, Christoph Hellwig wrote: > On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote: > > Not sure what you're even disagreeing with, as I *do* expect new > > filesystems to > > be held to a high standard, and to be written with the assumption that the > > on-disk data may be corrupted or malicious. We just can't expect the bar > > to be > > so high (e.g. no bugs) that it's never been attained by *any* filesystem > > even > > after years/decades of active development. If the developers were careful, > > the > > code generally looks robust, and they are willing to address such bugs as > > they > > are found, realistically that's as good as we can expect to get... > > Well, the impression I got from Richards quick look and the reply to it is > that there is very little attempt to validate the ondisk data structure > and there is absolutely no priority to do so. Which is very different > from there is a bug or two here and there. As my second reply to Richard, I didn't fuzz all the on-disk fields for EROFS. and as my reply to Richard / Greg, current EROFS is used on the top of dm-verity. I cannot say how well EROFS will be performed on malformed images (and you can also find the bug richard pointed out is a miswritten break->continue by myself). I posted the upstream EROFS post on July 4, 2019 and a month and a half later, no one can tell me (yes, thanks for kind people reply me about their suggestion) what we should do next (you can see these emails, I sent many times) to meet the minimal upstream requirements and rare people can even dip into my code. That is all I want to say. I will work on autofuzz these days, and I want to know how to meet your requirements on this (you can tell us your standard, how well should we do). OK, you don't reply to my post once, I have no idea how to get your first reply. Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
- Ursprüngliche Mail - > Von: "tytso" > An: "richard" > CC: "Greg Kroah-Hartman" , "Gao Xiang" > , "Jan Kara" , "Chao > Yu" , "Dave Chinner" , "David > Sterba" , "Miao Xie" > , "devel" , "Stephen > Rothwell" , "Darrick" > , "Christoph Hellwig" , "Amir > Goldstein" , > "linux-erofs" , "Al Viro" > , "Jaegeuk Kim" , > "linux-kernel" , "Li Guifu" > , "Fang Wei" , > "Pavel Machek" , "linux-fsdevel" > , "Andrew Morton" > , "torvalds" > Gesendet: Sonntag, 18. August 2019 19:46:21 > Betreff: Re: [PATCH] erofs: move erofs out of staging > On Sun, Aug 18, 2019 at 07:06:40PM +0200, Richard Weinberger wrote: >> > So holding a file system like EROFS to a higher standard than say, >> > ext4, xfs, or btrfs hardly seems fair. >> >> Nobody claimed that. > > Pointing out that erofs has issues in this area when Gao Xiang is > asking if erofs can be moved out of staging and join the "official > clubhouse" of file systems could certainly be reasonable interpreted > as such. Reporting such vulnerablities are a good thing, and > hopefully all file system maintainers will welcome them. Doing them > on a e-mail thread about promoting out of erofs is certainly going to > lead to inferences of a double standard. Well, this was not at all my intention. erofs raised my attention and instead of wasting a new thread I answered here and reported what I found while looking at it. That's all. Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote: > Not sure what you're even disagreeing with, as I *do* expect new filesystems > to > be held to a high standard, and to be written with the assumption that the > on-disk data may be corrupted or malicious. We just can't expect the bar to > be > so high (e.g. no bugs) that it's never been attained by *any* filesystem even > after years/decades of active development. If the developers were careful, > the > code generally looks robust, and they are willing to address such bugs as they > are found, realistically that's as good as we can expect to get... Well, the impression I got from Richards quick look and the reply to it is that there is very little attempt to validate the ondisk data structure and there is absolutely no priority to do so. Which is very different from there is a bug or two here and there. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 07:06:40PM +0200, Richard Weinberger wrote: > > So holding a file system like EROFS to a higher standard than say, > > ext4, xfs, or btrfs hardly seems fair. > > Nobody claimed that. Pointing out that erofs has issues in this area when Gao Xiang is asking if erofs can be moved out of staging and join the "official clubhouse" of file systems could certainly be reasonable interpreted as such. Reporting such vulnerablities are a good thing, and hopefully all file system maintainers will welcome them. Doing them on a e-mail thread about promoting out of erofs is certainly going to lead to inferences of a double standard. Cheers, - Ted ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 08:58:12AM -0700, Christoph Hellwig wrote: > On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote: > > Note that of the mainstream file systems, ext4 and xfs don't guarantee > > that it's safe to blindly take maliciously provided file systems, such > > as those provided by a untrusted container, and mount it on a file > > system without problems. As I recall, one of the XFS developers > > described file system fuzzing reports as a denial of service attack on > > the developers. > > I think this greatly misrepresents the general attitute of the XFS > developers. We take sanity checks for the modern v5 on disk format > very series, and put a lot of effort into handling corrupted file > systems as good as possible, although there are of course no guaranteeѕ. > > The quote that you've taken out of context is for the legacy v4 format > that has no checksums and other integrity features. Actually, what Prof. Kim's research group was doing was taking the latest file system formats (for ext4 and xfs) and fixing up the checksum after fuzzing the metadata blocks. The goal was to find potential security vulnerabilities, not to see if file systems would crash if fed invalid input. At least for ext4, at least one of Prof. Kim's fuzzing results was one that that I believe could have been leveraged into a stack overflow attack. I can't speak to his results with respect to XFS, since I didn't look at them. Cheers, - Ted ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 09:22:01AM -0700, Christoph Hellwig wrote: > On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote: > > Ted's observation was about maliciously-crafted filesystems, though, so > > integrity-only features such as metadata checksums are irrelevant. Also the > > filesystem version is irrelevant; anything accepted by the kernel code > > (even if > > I think allowing users to mount file systems (any of ours) without > privilege is a rather bad idea. But that doesn't mean we should not be > as robust as we can. Optionally disabling support for legacy formats > at compile and/or runtime is something we should actively look into as > well. > > > it's legacy/deprecated) is open attack surface. > > > > I personally consider it *mandatory* that we deal with this stuff. But I > > can > > understand that we don't do a good job at it, so we shouldn't hold a new > > filesystem to an unfairly high standard relative to other filesystems... > > I very much disagree. We can't really force anyone to fix up old file > systems. But we can very much hold new ones to (slightly) higher > standards. Thats the only way to get the average quality up. Some as > for things like code style - we can't magically fix up all old stuff, > but we can and usually do hold new code to higher standards. (Often not > to standards as high as I'd personally prefer, btw). Not sure what you're even disagreeing with, as I *do* expect new filesystems to be held to a high standard, and to be written with the assumption that the on-disk data may be corrupted or malicious. We just can't expect the bar to be so high (e.g. no bugs) that it's never been attained by *any* filesystem even after years/decades of active development. If the developers were careful, the code generally looks robust, and they are willing to address such bugs as they are found, realistically that's as good as we can expect to get... - Eric ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
- Ursprüngliche Mail - > So holding a file system like EROFS to a higher standard than say, > ext4, xfs, or btrfs hardly seems fair. Nobody claimed that. Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [GIT PULL] Staging/IIO driver fixes for 5.3-rc5
The pull request you sent on Sun, 18 Aug 2019 10:57:12 +0200: > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > tags/staging-5.3-rc5 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/ae1a616af36e5ad0726407b76feed5060a424744 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
Hi Hch, On Sun, Aug 18, 2019 at 09:22:01AM -0700, Christoph Hellwig wrote: > On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote: > > Ted's observation was about maliciously-crafted filesystems, though, so > > integrity-only features such as metadata checksums are irrelevant. Also the > > filesystem version is irrelevant; anything accepted by the kernel code > > (even if > > I think allowing users to mount file systems (any of ours) without > privilege is a rather bad idea. But that doesn't mean we should not be > as robust as we can. Optionally disabling support for legacy formats > at compile and/or runtime is something we should actively look into as > well. > > > it's legacy/deprecated) is open attack surface. > > > > I personally consider it *mandatory* that we deal with this stuff. But I > > can > > understand that we don't do a good job at it, so we shouldn't hold a new > > filesystem to an unfairly high standard relative to other filesystems... > > I very much disagree. We can't really force anyone to fix up old file > systems. But we can very much hold new ones to (slightly) higher > standards. Thats the only way to get the average quality up. Some as > for things like code style - we can't magically fix up all old stuff, > but we can and usually do hold new code to higher standards. (Often not > to standards as high as I'd personally prefer, btw). I personally don't want to discuss about other fses here... I think XFS developers do great jobs all the time and EROFS is a simple file system compared with these generic file systems. I can promise you that our team will fix bug reports in time, and I personally think the current EROFS code is not as bad as a bullsh**t... If you have some time, I'm very happy if you can take some of your precious time on our work... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote: > Ted's observation was about maliciously-crafted filesystems, though, so > integrity-only features such as metadata checksums are irrelevant. Also the > filesystem version is irrelevant; anything accepted by the kernel code (even > if I think allowing users to mount file systems (any of ours) without privilege is a rather bad idea. But that doesn't mean we should not be as robust as we can. Optionally disabling support for legacy formats at compile and/or runtime is something we should actively look into as well. > it's legacy/deprecated) is open attack surface. > > I personally consider it *mandatory* that we deal with this stuff. But I can > understand that we don't do a good job at it, so we shouldn't hold a new > filesystem to an unfairly high standard relative to other filesystems... I very much disagree. We can't really force anyone to fix up old file systems. But we can very much hold new ones to (slightly) higher standards. Thats the only way to get the average quality up. Some as for things like code style - we can't magically fix up all old stuff, but we can and usually do hold new code to higher standards. (Often not to standards as high as I'd personally prefer, btw). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 08:58:12AM -0700, Christoph Hellwig wrote: > On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote: > > Note that of the mainstream file systems, ext4 and xfs don't guarantee > > that it's safe to blindly take maliciously provided file systems, such > > as those provided by a untrusted container, and mount it on a file > > system without problems. As I recall, one of the XFS developers > > described file system fuzzing reports as a denial of service attack on > > the developers. > > I think this greatly misrepresents the general attitute of the XFS > developers. We take sanity checks for the modern v5 on disk format > very series, and put a lot of effort into handling corrupted file > systems as good as possible, although there are of course no guaranteeѕ. > > The quote that you've taken out of context is for the legacy v4 format > that has no checksums and other integrity features. Ted's observation was about maliciously-crafted filesystems, though, so integrity-only features such as metadata checksums are irrelevant. Also the filesystem version is irrelevant; anything accepted by the kernel code (even if it's legacy/deprecated) is open attack surface. I personally consider it *mandatory* that we deal with this stuff. But I can understand that we don't do a good job at it, so we shouldn't hold a new filesystem to an unfairly high standard relative to other filesystems... - Eric ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
Hi Ted, On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote: > On Sun, Aug 18, 2019 at 11:21:13AM +0200, Richard Weinberger wrote: > > > Not to say that erofs shouldn't be worked on to fix these kinds of > > > issues, just that it's not an unheard of thing to trust the disk image. > > > Especially for the normal usage model of erofs, where the whole disk > > > image is verfied before it is allowed to be mounted as part of the boot > > > process. > > > > For normal use I see no problem at all. > > I fear distros that try to mount anything you plug into your USB. > > > > At least SUSE already blacklists erofs: > > https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24 > > Note that of the mainstream file systems, ext4 and xfs don't guarantee > that it's safe to blindly take maliciously provided file systems, such > as those provided by a untrusted container, and mount it on a file > system without problems. As I recall, one of the XFS developers > described file system fuzzing reports as a denial of service attack on > the developers. And on the ext4 side, while I try to address them, it > is by no means considered a high priority work item, and I don't > consider fixes of fuzzing reports to be worthy of coordinated > disclosure or a high priority bug to fix. (I have closed more bugs in > this area than XFS has, although that may be that ext4 started with > more file system format bugs than XFS; however given the time to first > bug in 2017 using American Fuzzy Lop[1] being 5 seconds for btrfs, 10 > seconds for f2fs, 25 seconds for reiserfs, 4 minutes for ntfs, 1h45m > for xfs, and 2h for ext4, that seems unlikely.) > > [1] > https://events.static.linuxfound.org/sites/events/files/slides/AFL%20filesystem%20fuzzing%2C%20Vault%202016_0.pdf > > So holding a file system like EROFS to a higher standard than say, > ext4, xfs, or btrfs hardly seems fair. There seems to be a very > unfortunate tendancy for us to hold new file systems to impossibly > high standards, when in fact, adding a file system to Linux should > not, in my opinion, be a remarkable event. We have a huge number of > them already, many of which are barely maintained and probably have > far worse issues than file systems trying to get into the clubhouse. > > If a file system is requesting core changes to the VM or block layers, > sure, we should care about the interfaces. But this nitpicking about > whether or not a file system can be trusted in what I consider to be > COMPLETELY INSANE CONTAINER USE CASES is really not fair. Thanks for your kind reply and understanding... Yes, EROFS is now still like a little baby, and what I can do is to write more code to make it grown up... but I personally cannot write bug-free code all the time (because sometimes I could be in a low mood...) In the past year, we already adds many error handling path for corrupted images and handles all BUG_ONs in a more proper way... we are doing our best... Our team will continue focusing on all bug reports from external / internal sources and fix them all in time. and for more wider scenerios, I'd like to build an autofuzzer tools based on erofs-utils to make EROFS more strong as one of the next step. Thanks, Gao Xiang > > Cheers, > > - Ted ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote: > Note that of the mainstream file systems, ext4 and xfs don't guarantee > that it's safe to blindly take maliciously provided file systems, such > as those provided by a untrusted container, and mount it on a file > system without problems. As I recall, one of the XFS developers > described file system fuzzing reports as a denial of service attack on > the developers. I think this greatly misrepresents the general attitute of the XFS developers. We take sanity checks for the modern v5 on disk format very series, and put a lot of effort into handling corrupted file systems as good as possible, although there are of course no guaranteeѕ. The quote that you've taken out of context is for the legacy v4 format that has no checksums and other integrity features. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: android: Remove ion device tree bindings from the TODO
Commit 23a4388f24f5 ("staging: android: ion: Remove file ion_chunk_heap.c") and eadbf7a34e44 ("staging: android: ion: Remove file ion_carveout_heap.c") removed the chunk and carveout heaps from ion but left behind the device tree bindings for them in the TODO, this patch removes it. Signed-off-by: Donald Yandt --- Changes in v2: - Referenced previous commits and described the commit in greater detail. --- drivers/staging/android/TODO | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO index fbf015cc6d62..767dd98fd92d 100644 --- a/drivers/staging/android/TODO +++ b/drivers/staging/android/TODO @@ -6,8 +6,6 @@ TODO: ion/ - - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would - involve putting appropriate bindings in a memory node for Ion to find. - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0) - Better test framework (integration with VGEM was suggested) -- 2.21.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 11:21:13AM +0200, Richard Weinberger wrote: > > Not to say that erofs shouldn't be worked on to fix these kinds of > > issues, just that it's not an unheard of thing to trust the disk image. > > Especially for the normal usage model of erofs, where the whole disk > > image is verfied before it is allowed to be mounted as part of the boot > > process. > > For normal use I see no problem at all. > I fear distros that try to mount anything you plug into your USB. > > At least SUSE already blacklists erofs: > https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24 Note that of the mainstream file systems, ext4 and xfs don't guarantee that it's safe to blindly take maliciously provided file systems, such as those provided by a untrusted container, and mount it on a file system without problems. As I recall, one of the XFS developers described file system fuzzing reports as a denial of service attack on the developers. And on the ext4 side, while I try to address them, it is by no means considered a high priority work item, and I don't consider fixes of fuzzing reports to be worthy of coordinated disclosure or a high priority bug to fix. (I have closed more bugs in this area than XFS has, although that may be that ext4 started with more file system format bugs than XFS; however given the time to first bug in 2017 using American Fuzzy Lop[1] being 5 seconds for btrfs, 10 seconds for f2fs, 25 seconds for reiserfs, 4 minutes for ntfs, 1h45m for xfs, and 2h for ext4, that seems unlikely.) [1] https://events.static.linuxfound.org/sites/events/files/slides/AFL%20filesystem%20fuzzing%2C%20Vault%202016_0.pdf So holding a file system like EROFS to a higher standard than say, ext4, xfs, or btrfs hardly seems fair. There seems to be a very unfortunate tendancy for us to hold new file systems to impossibly high standards, when in fact, adding a file system to Linux should not, in my opinion, be a remarkable event. We have a huge number of them already, many of which are barely maintained and probably have far worse issues than file systems trying to get into the clubhouse. If a file system is requesting core changes to the VM or block layers, sure, we should care about the interfaces. But this nitpicking about whether or not a file system can be trusted in what I consider to be COMPLETELY INSANE CONTAINER USE CASES is really not fair. Cheers, - Ted ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8712: Improve naming of include hearder guards
Choose a better name for the include hearder guard used in rtl871x_io.h. '_IO_H_' is to generic and does not match the comment after the #endif. Use '_RTL871X_IO_H_' instead. Also make the comments in the #endif /* XXX */ match the name used in #ifndef. Signed-off-by: Christophe JAILLET --- __RTL871X_RF_H_ could have been turned into _RTL871X_RF_H_ (one leading '_' removed) but there does not seem to be a clear logic in the way the header guards are named in this directory. So leave it as-is in order to limit the diff. --- drivers/staging/rtl8712/rtl871x_io.h | 7 +++ drivers/staging/rtl8712/rtl871x_rf.h | 3 +-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/staging/rtl8712/rtl871x_io.h b/drivers/staging/rtl8712/rtl871x_io.h index 28941423b7ed..c20dd5a6bbd1 100644 --- a/drivers/staging/rtl8712/rtl871x_io.h +++ b/drivers/staging/rtl8712/rtl871x_io.h @@ -11,8 +11,8 @@ * Larry Finger * **/ -#ifndef _IO_H_ -#define _IO_H_ +#ifndef _RTL871X_IO_H_ +#define _RTL871X_IO_H_ #include "osdep_service.h" #include "osdep_intf.h" @@ -234,5 +234,4 @@ void r8712_write_port(struct _adapter *adapter, u32 addr, u32 cnt, u8 *pmem); uint r8712_alloc_io_queue(struct _adapter *adapter); void r8712_free_io_queue(struct _adapter *adapter); -#endif /*_RTL8711_IO_H_*/ - +#endif /*_RTL871X_IO_H_*/ diff --git a/drivers/staging/rtl8712/rtl871x_rf.h b/drivers/staging/rtl8712/rtl871x_rf.h index cc54453cd424..7d98921a48fa 100644 --- a/drivers/staging/rtl8712/rtl871x_rf.h +++ b/drivers/staging/rtl8712/rtl871x_rf.h @@ -52,5 +52,4 @@ enum { RTL8712_RFC_2T2R = 0x22 }; -#endif /*_RTL8711_RF_H_*/ - +#endif /*__RTL871X_RF_H_*/ -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()
On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote: > Hi Gao, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [cannot apply to v5.3-rc4 next-20190816] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] ... those patches should be applied to staging tree since linux-next has not been updated yet... Thanks, Gao Xiang > > url: > https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344 > config: arm64-allyesconfig (attached as .config) > compiler: aarch64-linux-gcc (GCC) 7.4.0 > reproduce: > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.4.0 make.cross ARCH=arm64 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot > > All errors (new ones prefixed by >>): > >drivers/staging/erofs/dir.c: In function 'erofs_readdir': > >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared > >> (first use in this function); did you mean 'FS_NRSUPER'? >err = -EFSCORRUPTED; > ^~~~ > FS_NRSUPER >drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is > reported only once for each function it appears in > > vim +110 drivers/staging/erofs/dir.c > > 85 > 86static int erofs_readdir(struct file *f, struct dir_context > *ctx) > 87{ > 88struct inode *dir = file_inode(f); > 89struct address_space *mapping = dir->i_mapping; > 90const size_t dirsize = i_size_read(dir); > 91unsigned int i = ctx->pos / EROFS_BLKSIZ; > 92unsigned int ofs = ctx->pos % EROFS_BLKSIZ; > 93int err = 0; > 94bool initial = true; > 95 > 96while (ctx->pos < dirsize) { > 97struct page *dentry_page; > 98struct erofs_dirent *de; > 99unsigned int nameoff, maxsize; >100 >101dentry_page = read_mapping_page(mapping, i, > NULL); >102if (dentry_page == ERR_PTR(-ENOMEM)) { >103errln("no memory to readdir of logical > block %u of nid %llu", >104 i, EROFS_V(dir)->nid); >105err = -ENOMEM; >106break; >107} else if (IS_ERR(dentry_page)) { >108errln("fail to readdir of logical block > %u of nid %llu", >109 i, EROFS_V(dir)->nid); > > 110err = -EFSCORRUPTED; >111break; >112} >113 >114de = (struct erofs_dirent *)kmap(dentry_page); >115 >116nameoff = le16_to_cpu(de->nameoff); >117 >118if (unlikely(nameoff < sizeof(struct > erofs_dirent) || >119 nameoff >= PAGE_SIZE)) { >120errln("%s, invalid de[0].nameoff %u", >121 __func__, nameoff); >122 >123err = -EIO; >124goto skip_this; >125} >126 >127maxsize = min_t(unsigned int, >128dirsize - ctx->pos + ofs, > PAGE_SIZE); >129 >130/* search dirents at the arbitrary position */ >131if (unlikely(initial)) { >132initial = false; >133 >134ofs = roundup(ofs, sizeof(struct > erofs_dirent)); >135if (unlikely(ofs >= nameoff)) >136goto skip_this; >137} >138 >139err = erofs_fill_dentries(ctx, de, , > nameoff, maxsize); >140skip_this: >14
Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()
Hi Gao, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc4 next-20190816] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344 config: arm64-allyesconfig (attached as .config) compiler: aarch64-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=arm64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/staging/erofs/dir.c: In function 'erofs_readdir': >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first >> use in this function); did you mean 'FS_NRSUPER'? err = -EFSCORRUPTED; ^~~~ FS_NRSUPER drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in vim +110 drivers/staging/erofs/dir.c 85 86 static int erofs_readdir(struct file *f, struct dir_context *ctx) 87 { 88 struct inode *dir = file_inode(f); 89 struct address_space *mapping = dir->i_mapping; 90 const size_t dirsize = i_size_read(dir); 91 unsigned int i = ctx->pos / EROFS_BLKSIZ; 92 unsigned int ofs = ctx->pos % EROFS_BLKSIZ; 93 int err = 0; 94 bool initial = true; 95 96 while (ctx->pos < dirsize) { 97 struct page *dentry_page; 98 struct erofs_dirent *de; 99 unsigned int nameoff, maxsize; 100 101 dentry_page = read_mapping_page(mapping, i, NULL); 102 if (dentry_page == ERR_PTR(-ENOMEM)) { 103 errln("no memory to readdir of logical block %u of nid %llu", 104i, EROFS_V(dir)->nid); 105 err = -ENOMEM; 106 break; 107 } else if (IS_ERR(dentry_page)) { 108 errln("fail to readdir of logical block %u of nid %llu", 109i, EROFS_V(dir)->nid); > 110 err = -EFSCORRUPTED; 111 break; 112 } 113 114 de = (struct erofs_dirent *)kmap(dentry_page); 115 116 nameoff = le16_to_cpu(de->nameoff); 117 118 if (unlikely(nameoff < sizeof(struct erofs_dirent) || 119 nameoff >= PAGE_SIZE)) { 120 errln("%s, invalid de[0].nameoff %u", 121__func__, nameoff); 122 123 err = -EIO; 124 goto skip_this; 125 } 126 127 maxsize = min_t(unsigned int, 128 dirsize - ctx->pos + ofs, PAGE_SIZE); 129 130 /* search dirents at the arbitrary position */ 131 if (unlikely(initial)) { 132 initial = false; 133 134 ofs = roundup(ofs, sizeof(struct erofs_dirent)); 135 if (unlikely(ofs >= nameoff)) 136 goto skip_this; 137 } 138 139 err = erofs_fill_dentries(ctx, de, , nameoff, maxsize); 140 skip_this: 141 kunmap(dentry_page); 142 143 put_page(dentry_page); 144 145 ctx->pos = blknr_to_addr(i) + ofs; 146 147 if (unlikely(err)) 148 break; 149 ++i; 150 ofs = 0; 151 } 152 return err < 0 ? err : 0; 153 } 154 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: refuse to mount images with malformed volume name
On 2019-8-18 18:28, Gao Xiang wrote: > From: Gao Xiang > > As Richard reminder [1], A valid volume name should be > ended in NIL terminator within the length of volume_name. > > Since this field currently isn't really used, let's fix > it to avoid potential bugs in the future. > > [1] > https://lore.kernel.org/r/1133002215.69049.1566119033047.javamail.zim...@nod.at/ > > Reported-by: Richard Weinberger > Signed-off-by: Gao Xiang Reviewed-by: Chao Yu Thanks, ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4] staging: erofs: fix an error handling in erofs_readdir()
From: Gao Xiang Richard observed a forever loop of erofs_read_raw_page() [1] which can be generated by forcely setting ->u.i_blkaddr to 0xdeadbeef (as my understanding block layer can handle access beyond end of device correctly). After digging into that, it seems the problem is highly related with directories and then I found the root cause is an improper error handling in erofs_readdir(). Let's fix it now. [1] https://lore.kernel.org/r/1163995781.68824.1566084358245.javamail.zim...@nod.at/ Reported-by: Richard Weinberger Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") Cc: # 4.19+ Reviewed-by: Chao Yu Signed-off-by: Gao Xiang --- changelog from v3: - kill message when memory allocation fails as suggested by Matthew; [RESEND] --> add the missing v3 version in subject, no logic change. changelog from v2: - transform EIO to EFSCORRUPTED as suggested by Matthew; changelog from v1: - fix the incorrect external link in commit message. This patch is based on staging-testing tree and https://lore.kernel.org/r/20190817082313.21040-1-hsiang...@aol.com/ can still be properly applied after this patch. drivers/staging/erofs/dir.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c index 5f38382637e6..77ef856df9f3 100644 --- a/drivers/staging/erofs/dir.c +++ b/drivers/staging/erofs/dir.c @@ -82,8 +82,15 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) unsigned int nameoff, maxsize; dentry_page = read_mapping_page(mapping, i, NULL); - if (IS_ERR(dentry_page)) - continue; + if (dentry_page == ERR_PTR(-ENOMEM)) { + err = -ENOMEM; + break; + } else if (IS_ERR(dentry_page)) { + errln("fail to readdir of logical block %u of nid %llu", + i, EROFS_V(dir)->nid); + err = -EFSCORRUPTED; + break; + } de = (struct erofs_dirent *)kmap(dentry_page); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()
On Sun, Aug 18, 2019 at 05:33:14AM -0700, Matthew Wilcox wrote: > On Sun, Aug 18, 2019 at 11:21:11AM +0800, Gao Xiang wrote: > > + if (dentry_page == ERR_PTR(-ENOMEM)) { > > + errln("no memory to readdir of logical block %u of nid > > %llu", > > + i, EROFS_V(dir)->nid); > > I don't think you need the error message. If we get a memory allocation > failure, there's already going to be a lot of spew in the logs from the > mm system. And if we do fail to allocate memory, we don't need to know > the logical block number or the nid -- it has nothiing to do with those; > the system simply ran out of memory. OK, I agree with you. There is a messy of messages when memory allocation fail. Since I don't really care apart from crashing or hanging the kernel, I will resend the patch to make you and Chao happy... :) Thanks, Gao Xiang > > > + err = -ENOMEM; > > + break; > > + } else if (IS_ERR(dentry_page)) { > > + errln("fail to readdir of logical block %u of nid %llu", > > + i, EROFS_V(dir)->nid); > > + err = -EFSCORRUPTED; > > + break; > > + } > > > > de = (struct erofs_dirent *)kmap(dentry_page); > > > > -- > > 2.17.1 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()
On Sun, Aug 18, 2019 at 11:21:11AM +0800, Gao Xiang wrote: > + if (dentry_page == ERR_PTR(-ENOMEM)) { > + errln("no memory to readdir of logical block %u of nid > %llu", > + i, EROFS_V(dir)->nid); I don't think you need the error message. If we get a memory allocation failure, there's already going to be a lot of spew in the logs from the mm system. And if we do fail to allocate memory, we don't need to know the logical block number or the nid -- it has nothiing to do with those; the system simply ran out of memory. > + err = -ENOMEM; > + break; > + } else if (IS_ERR(dentry_page)) { > + errln("fail to readdir of logical block %u of nid %llu", > + i, EROFS_V(dir)->nid); > + err = -EFSCORRUPTED; > + break; > + } > > de = (struct erofs_dirent *)kmap(dentry_page); > > -- > 2.17.1 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()
On 2019-8-18 11:21, Gao Xiang wrote: > From: Gao Xiang > > Richard observed a forever loop of erofs_read_raw_page() [1] > which can be generated by forcely setting ->u.i_blkaddr > to 0xdeadbeef (as my understanding block layer can > handle access beyond end of device correctly). > > After digging into that, it seems the problem is highly > related with directories and then I found the root cause > is an improper error handling in erofs_readdir(). > > Let's fix it now. > > [1] > https://lore.kernel.org/r/1163995781.68824.1566084358245.javamail.zim...@nod.at/ > > Reported-by: Richard Weinberger > Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") > Cc: # 4.19+ > Signed-off-by: Gao Xiang Reviewed-by: Chao Yu Thanks, ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()
Hi Xiang, On 2019-8-18 18:52, Gao Xiang wrote: > Hi Chao, > > On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote: >> On 2019-8-18 10:53, Matthew Wilcox wrote: >>> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote: On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote: > On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote: >> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct >> dir_context *ctx) >> unsigned int nameoff, maxsize; >> >> dentry_page = read_mapping_page(mapping, i, NULL); >> -if (IS_ERR(dentry_page)) >> -continue; >> +if (IS_ERR(dentry_page)) { >> +errln("fail to readdir of logical block %u of >> nid %llu", >> + i, EROFS_V(dir)->nid); >> +err = PTR_ERR(dentry_page); >> +break; > > I don't think you want to use the errno that came back from > read_mapping_page() (which is, I think, always going to be -EIO). > Rather you want -EFSCORRUPTED, at least if I understand the recent > patches to ext2/ext4/f2fs/xfs/... Thanks for your reply and noticing this. :) Yes, as I talked with you about read_mapping_page() in a xfs related topic earlier, I think I fully understand what returns here. I actually had some concern about that before sending out this patch. You know the status is PG_uptodate is not set and PG_error is set here. But we cannot know it is actually a disk read error or due to corrupted images (due to lack of page flags or some status, and I think it could be a waste of page structure space for such corrupted image or disk error)... And some people also like propagate errors from insiders... (and they could argue about err = -EFSCORRUPTED as well..) I'd like hear your suggestion about this after my words above? still return -EFSCORRUPTED? >>> >>> I don't think it matters whether it's due to a disk error or a corrupted >>> image. We can't read the directory entry, so we should probably return >>> -EFSCORRUPTED. Thinking about it some more, read_mapping_page() can >>> also return -ENOMEM, so it should probably look something like this: >>> >>> err = 0; >>> if (dentry_page == ERR_PTR(-ENOMEM)) >>> err = -ENOMEM; >>> else if (IS_ERR(dentry_page)) { >>> errln("fail to readdir of logical block %u of nid %llu", >>> i, EROFS_V(dir)->nid); >>> err = -EFSCORRUPTED; >> >> Well, if there is real IO error happen under filesystem, we should return >> -EIO >> instead of EFSCORRUPTED? >> >> The right fix may be that doing sanity check on on-disk blkaddr, and return >> -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller >> erofs_readdir(), IIUC below error info. > > In my thought, I actually don't care what is actually returned > (In other words, I have no tendency about EFSCORRUPTED / EIO) > as long as it behaves normal for corrupted image. I doubt that user can and be willing to handle different error code in there error path. > > A little concern is that I have no idea whether all user problems > can handle EUCLEAN properly. Yes, I can see it's widely used in fileystem, I guess it would be better to update manual of common fs interfaces to let user be aware of the meaning of it. > > I don't want to limit blkaddr as what ->blocks recorded in > erofs_super_block since it's already used for our hotpatching > approach and that field is only used for statfs() for users > to know its visible size, and block layer will block such > invalid block access. > > All in all, that is minor. I think we can fix as what Matthew said. Yeah, as we discuss offline, we need keep flexibility on current version for android, and maybe we can add a feature to check blkaddr validation later. Thanks, > > Thanks, > Gao Xiang > >> >>> [36297.354090] attempt to access beyond end of device >>> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128 >>> [36297.354107] attempt to access beyond end of device >>> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128 >>> [36301.827234] attempt to access beyond end of device >>> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128 >> >> Thanks, >> >>> } >>> >>> if (err) >>> break; >>> ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: refuse to mount images with malformed volume name
Hi Gao, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc4 next-20190816] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-refuse-to-mount-images-with-malformed-volume-name/20190818-193037 config: sparc64-allmodconfig (attached as .config) compiler: sparc64-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=sparc64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/staging/erofs/super.c: In function 'superblock_read': >> drivers/staging/erofs/super.c:153:10: error: 'EFSCORRUPTED' undeclared >> (first use in this function); did you mean 'FS_NRSUPER'? ret = -EFSCORRUPTED; ^~~~ FS_NRSUPER drivers/staging/erofs/super.c:153:10: note: each undeclared identifier is reported only once for each function it appears in vim +153 drivers/staging/erofs/super.c 140 141 sbi->root_nid = le16_to_cpu(layout->root_nid); 142 sbi->inos = le64_to_cpu(layout->inos); 143 144 sbi->build_time = le64_to_cpu(layout->build_time); 145 sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec); 146 147 memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid)); 148 149 ret = strscpy(sbi->volume_name, layout->volume_name, 150sizeof(layout->volume_name)); 151 if (ret < 0) { /* -E2BIG */ 152 errln("bad volume name without NIL terminator"); > 153 ret = -EFSCORRUPTED; 154 goto out; 155 } 156 ret = 0; 157 out: 158 brelse(bh); 159 return ret; 160 } 161 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()
Hi Gao, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc4 next-20190816] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344 config: sparc64-allmodconfig (attached as .config) compiler: sparc64-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=sparc64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/staging//erofs/dir.c: In function 'erofs_readdir': >> drivers/staging//erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first >> use in this function) err = -EFSCORRUPTED; ^~~~ drivers/staging//erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in vim +/EFSCORRUPTED +110 drivers/staging//erofs/dir.c 85 86 static int erofs_readdir(struct file *f, struct dir_context *ctx) 87 { 88 struct inode *dir = file_inode(f); 89 struct address_space *mapping = dir->i_mapping; 90 const size_t dirsize = i_size_read(dir); 91 unsigned int i = ctx->pos / EROFS_BLKSIZ; 92 unsigned int ofs = ctx->pos % EROFS_BLKSIZ; 93 int err = 0; 94 bool initial = true; 95 96 while (ctx->pos < dirsize) { 97 struct page *dentry_page; 98 struct erofs_dirent *de; 99 unsigned int nameoff, maxsize; 100 101 dentry_page = read_mapping_page(mapping, i, NULL); 102 if (dentry_page == ERR_PTR(-ENOMEM)) { 103 errln("no memory to readdir of logical block %u of nid %llu", 104i, EROFS_V(dir)->nid); 105 err = -ENOMEM; 106 break; 107 } else if (IS_ERR(dentry_page)) { 108 errln("fail to readdir of logical block %u of nid %llu", 109i, EROFS_V(dir)->nid); > 110 err = -EFSCORRUPTED; 111 break; 112 } 113 114 de = (struct erofs_dirent *)kmap(dentry_page); 115 116 nameoff = le16_to_cpu(de->nameoff); 117 118 if (unlikely(nameoff < sizeof(struct erofs_dirent) || 119 nameoff >= PAGE_SIZE)) { 120 errln("%s, invalid de[0].nameoff %u", 121__func__, nameoff); 122 123 err = -EIO; 124 goto skip_this; 125 } 126 127 maxsize = min_t(unsigned int, 128 dirsize - ctx->pos + ofs, PAGE_SIZE); 129 130 /* search dirents at the arbitrary position */ 131 if (unlikely(initial)) { 132 initial = false; 133 134 ofs = roundup(ofs, sizeof(struct erofs_dirent)); 135 if (unlikely(ofs >= nameoff)) 136 goto skip_this; 137 } 138 139 err = erofs_fill_dentries(ctx, de, , nameoff, maxsize); 140 skip_this: 141 kunmap(dentry_page); 142 143 put_page(dentry_page); 144 145 ctx->pos = blknr_to_addr(i) + ofs; 146 147 if (unlikely(err)) 148 break; 149 ++i; 150 ofs = 0; 151 } 152 return err < 0 ? err : 0; 153 } 154 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()
Hi Chao, On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote: > On 2019-8-18 10:53, Matthew Wilcox wrote: > > On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote: > >> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote: > >>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote: > @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct > dir_context *ctx) > unsigned int nameoff, maxsize; > > dentry_page = read_mapping_page(mapping, i, NULL); > -if (IS_ERR(dentry_page)) > -continue; > +if (IS_ERR(dentry_page)) { > +errln("fail to readdir of logical block %u of > nid %llu", > + i, EROFS_V(dir)->nid); > +err = PTR_ERR(dentry_page); > +break; > >>> > >>> I don't think you want to use the errno that came back from > >>> read_mapping_page() (which is, I think, always going to be -EIO). > >>> Rather you want -EFSCORRUPTED, at least if I understand the recent > >>> patches to ext2/ext4/f2fs/xfs/... > >> > >> Thanks for your reply and noticing this. :) > >> > >> Yes, as I talked with you about read_mapping_page() in a xfs related > >> topic earlier, I think I fully understand what returns here. > >> > >> I actually had some concern about that before sending out this patch. > >> You know the status is > >>PG_uptodate is not set and PG_error is set here. > >> > >> But we cannot know it is actually a disk read error or due to > >> corrupted images (due to lack of page flags or some status, and > >> I think it could be a waste of page structure space for such > >> corrupted image or disk error)... > >> > >> And some people also like propagate errors from insiders... > >> (and they could argue about err = -EFSCORRUPTED as well..) > >> > >> I'd like hear your suggestion about this after my words above? > >> still return -EFSCORRUPTED? > > > > I don't think it matters whether it's due to a disk error or a corrupted > > image. We can't read the directory entry, so we should probably return > > -EFSCORRUPTED. Thinking about it some more, read_mapping_page() can > > also return -ENOMEM, so it should probably look something like this: > > > > err = 0; > > if (dentry_page == ERR_PTR(-ENOMEM)) > > err = -ENOMEM; > > else if (IS_ERR(dentry_page)) { > > errln("fail to readdir of logical block %u of nid %llu", > > i, EROFS_V(dir)->nid); > > err = -EFSCORRUPTED; > > Well, if there is real IO error happen under filesystem, we should return -EIO > instead of EFSCORRUPTED? > > The right fix may be that doing sanity check on on-disk blkaddr, and return > -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller > erofs_readdir(), IIUC below error info. In my thought, I actually don't care what is actually returned (In other words, I have no tendency about EFSCORRUPTED / EIO) as long as it behaves normal for corrupted image. A little concern is that I have no idea whether all user problems can handle EUCLEAN properly. I don't want to limit blkaddr as what ->blocks recorded in erofs_super_block since it's already used for our hotpatching approach and that field is only used for statfs() for users to know its visible size, and block layer will block such invalid block access. All in all, that is minor. I think we can fix as what Matthew said. Thanks, Gao Xiang > > > [36297.354090] attempt to access beyond end of device > > [36297.354098] loop17: rw=0, want=29887428984, limit=1953128 > > [36297.354107] attempt to access beyond end of device > > [36297.354109] loop17: rw=0, want=29887428480, limit=1953128 > > [36301.827234] attempt to access beyond end of device > > [36301.827243] loop17: rw=0, want=29887428480, limit=1953128 > > Thanks, > > > } > > > > if (err) > > break; > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()
On 2019-8-18 10:53, Matthew Wilcox wrote: > On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote: >> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote: >>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote: @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) unsigned int nameoff, maxsize; dentry_page = read_mapping_page(mapping, i, NULL); - if (IS_ERR(dentry_page)) - continue; + if (IS_ERR(dentry_page)) { + errln("fail to readdir of logical block %u of nid %llu", +i, EROFS_V(dir)->nid); + err = PTR_ERR(dentry_page); + break; >>> >>> I don't think you want to use the errno that came back from >>> read_mapping_page() (which is, I think, always going to be -EIO). >>> Rather you want -EFSCORRUPTED, at least if I understand the recent >>> patches to ext2/ext4/f2fs/xfs/... >> >> Thanks for your reply and noticing this. :) >> >> Yes, as I talked with you about read_mapping_page() in a xfs related >> topic earlier, I think I fully understand what returns here. >> >> I actually had some concern about that before sending out this patch. >> You know the status is >>PG_uptodate is not set and PG_error is set here. >> >> But we cannot know it is actually a disk read error or due to >> corrupted images (due to lack of page flags or some status, and >> I think it could be a waste of page structure space for such >> corrupted image or disk error)... >> >> And some people also like propagate errors from insiders... >> (and they could argue about err = -EFSCORRUPTED as well..) >> >> I'd like hear your suggestion about this after my words above? >> still return -EFSCORRUPTED? > > I don't think it matters whether it's due to a disk error or a corrupted > image. We can't read the directory entry, so we should probably return > -EFSCORRUPTED. Thinking about it some more, read_mapping_page() can > also return -ENOMEM, so it should probably look something like this: > > err = 0; > if (dentry_page == ERR_PTR(-ENOMEM)) > err = -ENOMEM; > else if (IS_ERR(dentry_page)) { > errln("fail to readdir of logical block %u of nid %llu", > i, EROFS_V(dir)->nid); > err = -EFSCORRUPTED; Well, if there is real IO error happen under filesystem, we should return -EIO instead of EFSCORRUPTED? The right fix may be that doing sanity check on on-disk blkaddr, and return -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller erofs_readdir(), IIUC below error info. > [36297.354090] attempt to access beyond end of device > [36297.354098] loop17: rw=0, want=29887428984, limit=1953128 > [36297.354107] attempt to access beyond end of device > [36297.354109] loop17: rw=0, want=29887428480, limit=1953128 > [36301.827234] attempt to access beyond end of device > [36301.827243] loop17: rw=0, want=29887428480, limit=1953128 Thanks, > } > > if (err) > break; > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: refuse to mount images with malformed volume name
From: Gao Xiang As Richard reminder [1], A valid volume name should be ended in NIL terminator within the length of volume_name. Since this field currently isn't really used, let's fix it to avoid potential bugs in the future. [1] https://lore.kernel.org/r/1133002215.69049.1566119033047.javamail.zim...@nod.at/ Reported-by: Richard Weinberger Signed-off-by: Gao Xiang --- drivers/staging/erofs/super.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c index f65a1ff9f42f..2da471010a86 100644 --- a/drivers/staging/erofs/super.c +++ b/drivers/staging/erofs/super.c @@ -131,9 +131,14 @@ static int superblock_read(struct super_block *sb) sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec); memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid)); - memcpy(sbi->volume_name, layout->volume_name, - sizeof(layout->volume_name)); + ret = strscpy(sbi->volume_name, layout->volume_name, + sizeof(layout->volume_name)); + if (ret < 0) { /* -E2BIG */ + errln("bad volume name without NIL terminator"); + ret = -EFSCORRUPTED; + goto out; + } ret = 0; out: brelse(bh); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
Hi Richard, On 2019-8-18 17:21, Richard Weinberger wrote: > For normal use I see no problem at all. > I fear distros that try to mount anything you plug into your USB. > > At least SUSE already blacklists erofs: > https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24 Thanks for letting us know current status of erofs in SUSE distro. Currently erofs cares more about the requirement of Android, in there, we are safe on fuzzed image case as dm-verity can keep all partition data being verified before mount. For other scenarios, like distro, erofs should improve itself step by step as many mainline filesystems in many aspects to fit the there-in requirement. :) Thanks, > > Thanks, > //richard > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 5:51 PM Oleksij Rempel wrote: > > lets see more code: > drivers/staging/mt7621-mmc/sd.c > /* clock source for host: global */ > #if defined(CONFIG_SOC_MT7620) > static u32 hclks[] = {4800}; /* +/- by chhung */ > #elif defined(CONFIG_SOC_MT7621) > static u32 hclks[] = {5000}; /* +/- by chhung */ > #endif > > hm.. 50Mhz again. Feels like APB clock. > > ./drivers/staging/mt7621-dts/mt7621.dtsi > cpuclock: cpuclock@0 { > #clock-cells = <0>; > compatible = "fixed-clock"; > > /* FIXME: there should be way to detect this */ > clock-frequency = <88000>; > }; > > Your driver is trying to cover cpuclock > > sysclock: sysclock@0 { > #clock-cells = <0>; > compatible = "fixed-clock"; > > /* This is normally 1/4 of cpuclock */ > clock-frequency = <22000>; > }; > > and most probably system clock alias "bus clock", most probably AHB. This sysclock was the 50MHz clock in OpenWrt. It's set as "bus clock" upstream by an incorrect commit. As already stated in previous reply: I'm not going to make assumption about clock plan using OpenWrt's device tree because it already contains several mistakes on clocks. Since the upstream device tree comes from there, I don't trust it either. You might want to check out patch 6/6 in this series where the original author of this commit in openwrt fixed some clocks and I ported it here. > [...] > > This debate goes nowhere. I've clarified the situation and made my > > point. Of course I can't read through the ancient and heavily hacked > > vendor kernel to figure out a clock plan myself. > > Ok, I provided you some productive technical hints how it should be > done. I don't think mt7620 has better documentation then mt7621 and even > in this case it was possible to make more or less good driver. It does. A clock plan for mt7620 is available in MT7620 Programming Guide, Page 14. Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 18.08.19 um 10:44 schrieb Chuanhong Guo: > On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo wrote: >> >> Hi! >> >> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel >> wrote: >>> >>> Am 18.08.19 um 09:19 schrieb Chuanhong Guo: Hi! On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel wrote: > >>> We have at least 2 know registers: >>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped >>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?). >>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for >>> all or some ip cores. >>> What is probably missing is a set of dividers for >>> each ip core. From your words it is not document. >> >> The specific missing part I was referring to, is parent clocks for >> every gates. I'm not going to assume this with current openwrt device >> tree because some peripherals doesn't have a clock binding at all or >> have a dummy one there. > > Ok, then I do not understand what is the motivation to upstream > something what is not nearly ready for use. Why isn't it "ready for use" then? A complete mt7621-pll driver will contain two parts: 1. A clock provider which outputs several clocks 2. A clock gate with parent clocks properly configured Two clocks provided here are just two clocks that can't be controlled in kernel no matter where it goes (arch/mips/ralink or drivers/clk). Having a working CPU clock provider is better than defining a fixed clock in dts because CPU clock can be controlled by bootloader. (BTW description for CPU PLL register is also missing in datasheet.) Clock gate is an unrelated part and there is no information to properly implement it unless MTK decided to release a clock plan somehow. >>> >>> With other words, your complete system is running with unknown clock >>> rates. >> >> And without this patchset the complete system is running with unknown >> clock and, even worse, we make assumptions about what clock bootloader >> uses, hardcoded it in dts and hope it is the correct value. >> >>> The source clock in the clock three can be configured differently >>> by bootloader but you don't know how it is done how and it is not >>> documented. >> >> Actually, I don't know about this and I didn't wrote the original >> clock calculation code. I just ported it from downstream OpenWrt >> kernel. Here's a piece of code from Mediatek's SDK kernel: >> >> case 0: >> reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44)); >> cpu_fdiv = ((reg >> 8) & 0x1F); >> cpu_ffrac = (reg & 0x1F); >> mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000; >> break; >> case 1: //CPU PLL >> reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648)); >> fbdiv = ((reg >> 4) & 0x7F) + 1; >> reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10)); >> reg = (reg >> 6) & 0x7; >> if(reg >= 6) { //25Mhz Xtal >> mips_cpu_feq = 25 * fbdiv * 1000 * 1000; >> } else if(reg >=3) { //40Mhz Xtal >> mips_cpu_feq = 20 * fbdiv * 1000 * 1000; >> } else { // 20Mhz Xtal >> /* TODO */ >> } >> break; >> >> >> >>> > This code is currently on prototyping phase Code for clock calculation is done, not "prototyping". > It means, we cannot expect that this driver will be fixed any time soon. I think clock gating is a separated feature instead of a broken part that has to be fixed. >>> >>> Ok, i would agree with it. But from what you said, this feature will be >>> never implemented. >>> >>> So, I repeat my question. What is the point to upstream code for a >>> system, which has not enough information to get proper clock rate even >>> for uart? or is uart running with cpu or bus clock rate? >> >> uart runs of a fixed 50MHz clock according to another piece of code >> from MTK SDK: >> (a pastebin version here for better readability. This specific >> question has nothing to do with patch reviewing and doesn't need to be >> preserved in mail forever.) >> https://paste.ubuntu.com/p/fYmtDFW9nh/ ok. lets see more code: drivers/staging/mt7621-mmc/sd.c /* clock source for host: global */ #if defined(CONFIG_SOC_MT7620) static u32 hclks[] = {4800}; /* +/- by chhung */ #elif defined(CONFIG_SOC_MT7621) static u32 hclks[] = {5000}; /* +/- by chhung */ #endif hm.. 50Mhz again. Feels like APB clock. ./drivers/staging/mt7621-dts/mt7621.dtsi cpuclock: cpuclock@0 { #clock-cells = <0>; compatible = "fixed-clock"; /* FIXME: there should be way to detect this */ clock-frequency = <88000>; }; Your driver is trying to cover cpuclock sysclock: sysclock@0 { #clock-cells = <0>; compatible = "fixed-clock"; /* This is normally 1/4 of
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 11:03:53AM +0200, Richard Weinberger wrote: > - Urspr??ngliche Mail - > > I agree with you, but what can we do now is trying our best to fuzz > > all the fields. > > > > So, what is your opinion about EROFS? > > All I'm saying is that you should not blindly trust the disk. I completely agree with you, and I'm teaching EROFS to make the little naughty boy more strong... (we already have many error handling code, but I think I will teach him more, yes.) > > Another thing that raises my attention is in superblock_read(): > memcpy(sbi->volume_name, layout->volume_name, >sizeof(layout->volume_name)); > > Where do you check whether ->volume_name has a NUL terminator? > Currently this field has no user, maybe will add a check upon usage. > But this kind of things makes me wonder. Yes, I think this is a good point. :) Since volume_name is not used currently, I will fix it when it use late. I will make a note here (or more straightforward, I will fix it to avoid potential bug now.) Thanks, Gao Xiang > > Thanks, > //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
- Ursprüngliche Mail - > You have looked at reiserfs lately, right? :) Don't remind me of that. ;-) > Not to say that erofs shouldn't be worked on to fix these kinds of > issues, just that it's not an unheard of thing to trust the disk image. > Especially for the normal usage model of erofs, where the whole disk > image is verfied before it is allowed to be mounted as part of the boot > process. For normal use I see no problem at all. I fear distros that try to mount anything you plug into your USB. At least SUSE already blacklists erofs: https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24 Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()y
On Sun, Aug 18, 2019 at 05:10:38PM +0800, Gao Xiang via Linux-erofs wrote: > Hi Richard, > > On Sun, Aug 18, 2019 at 10:33:33AM +0200, Richard Weinberger wrote: > > - Urspr?ngliche Mail - > > > changelog from v2: > > > - transform EIO to EFSCORRUPTED as suggested by Matthew; > > > > erofs does not define EFSCORRUPTED, so the build fails. > > At least on Linus' tree as of today. > > Thanks for your reply :) > > I write all patches based on staging tree and do more cleanups further > than Linus' tree, EFSCORRUPTED was already introduced by Pavel days before... Sorry, I mean "introduced which was suggested by Paval"... > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing=a6b9b1d5eae61a68085030e50d56265dec5baa94 > > which can be fetched from > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git -b > staging-next > > Thanks, > Gao Xiang > > > > > Thanks, > > //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()
Hi Richard, On Sun, Aug 18, 2019 at 10:33:33AM +0200, Richard Weinberger wrote: > - Urspr?ngliche Mail - > > changelog from v2: > > - transform EIO to EFSCORRUPTED as suggested by Matthew; > > erofs does not define EFSCORRUPTED, so the build fails. > At least on Linus' tree as of today. Thanks for your reply :) I write all patches based on staging tree and do more cleanups further than Linus' tree, EFSCORRUPTED was already introduced by Pavel days before... https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing=a6b9b1d5eae61a68085030e50d56265dec5baa94 which can be fetched from git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git -b staging-next Thanks, Gao Xiang > > Thanks, > //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 11:03:53AM +0200, Richard Weinberger wrote: > - Ursprüngliche Mail - > > I agree with you, but what can we do now is trying our best to fuzz > > all the fields. > > > > So, what is your opinion about EROFS? > > All I'm saying is that you should not blindly trust the disk. > > Another thing that raises my attention is in superblock_read(): > memcpy(sbi->volume_name, layout->volume_name, >sizeof(layout->volume_name)); > > Where do you check whether ->volume_name has a NUL terminator? > Currently this field has no user, maybe will add a check upon usage. > But this kind of things makes me wonder. You have looked at reiserfs lately, right? :) Not to say that erofs shouldn't be worked on to fix these kinds of issues, just that it's not an unheard of thing to trust the disk image. Especially for the normal usage model of erofs, where the whole disk image is verfied before it is allowed to be mounted as part of the boot process. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
- Ursprüngliche Mail - > I agree with you, but what can we do now is trying our best to fuzz > all the fields. > > So, what is your opinion about EROFS? All I'm saying is that you should not blindly trust the disk. Another thing that raises my attention is in superblock_read(): memcpy(sbi->volume_name, layout->volume_name, sizeof(layout->volume_name)); Where do you check whether ->volume_name has a NUL terminator? Currently this field has no user, maybe will add a check upon usage. But this kind of things makes me wonder. Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[GIT PULL] Staging/IIO driver fixes for 5.3-rc5
The following changes since commit d45331b00ddb179e291766617259261c112db872: Linux 5.3-rc4 (2019-08-11 13:26:41 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git tags/staging-5.3-rc5 for you to fetch changes up to 48b30e10bfc20ec6195642cc09ea6f08a8015df7: Merge tag 'iio-fixes-for-5.3b' of git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio into staging-linus (2019-08-12 22:47:59 +0200) Staging/IIO fixes for 5.3-rc5 Here are 4 small staging and iio driver fixes for 5.3-rc5 Two are for the dt3000 comedi driver for some reported problems found in that codebase, and two are some small iio fixes. All of these have been in linux-next this week with no reported issues. Signed-off-by: Greg Kroah-Hartman Greg Kroah-Hartman (1): Merge tag 'iio-fixes-for-5.3b' of git://git.kernel.org/.../jic23/iio into staging-linus Ian Abbott (2): staging: comedi: dt3000: Fix signed integer overflow 'divider * base' staging: comedi: dt3000: Fix rounding up of timer divisor Jacopo Mondi (1): iio: adc: max9611: Fix temperature reading in probe Nuno Sá (1): iio: frequency: adf4371: Fix output frequency setting drivers/iio/adc/max9611.c | 2 +- drivers/iio/frequency/adf4371.c | 8 drivers/staging/comedi/drivers/dt3000.c | 8 3 files changed, 9 insertions(+), 9 deletions(-) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
On Sun, Aug 18, 2019 at 10:16:50AM +0200, Richard Weinberger wrote: > - Urspr?ngliche Mail - > >> While digging a little into the code I noticed that you have very few > >> checks of the on-disk data. > >> For example ->u.i_blkaddr. I gave it a try and created a > >> malformed filesystem where u.i_blkaddr is 0xdeadbeef, it causes the kernel > >> to loop forever around erofs_read_raw_page(). > > > > I don't fuzz all the on-disk fields for EROFS, I will do later.. > > You can see many in-kernel filesystems are still hardening the related > > stuff. Anyway, I will dig into this field you mentioned recently, but > > I think it can be fixed easily later. > > This is no excuse to redo all these bugs. :-) I agree with you, but what can we do now is trying our best to fuzz all the fields. So, what is your opinion about EROFS? > > I know that many in-kernel filesystems trust the disk ultimately, this is a > problem and huge attack vector. I EROFS already has many error handing paths to recover corrupted images, and your discovery is a bug out of one error handing path miswritten by me. I cannot make a guarantee that all the new things (every new kernel version will introduce new feature / new code) are bug-free since I am not a machine or code parser. My answer about this EROFS will be more stable with our development, we have a dedicated team with paid job, and since we currently use EROFS on the top of dm-verity for current Android, which will keep us from corrupted images. But yes, we will focus on fuzzing all the images for generic usages, and we will backport all these patches to old stable versions. Thanks, Gao Xiang > > Thanks, > //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo wrote: > > Hi! > > On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel wrote: > > > > Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > > > Hi! > > > > > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel > > > wrote: > > >> > > We have at least 2 know registers: > > SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped > > refclock. PLL and dividers used for CPU and some sort of BUS (AHB?). > > SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for > > all or some ip cores. > > What is probably missing is a set of dividers for > > each ip core. From your words it is not document. > > >>> > > >>> The specific missing part I was referring to, is parent clocks for > > >>> every gates. I'm not going to assume this with current openwrt device > > >>> tree because some peripherals doesn't have a clock binding at all or > > >>> have a dummy one there. > > >> > > >> Ok, then I do not understand what is the motivation to upstream > > >> something what is not nearly ready for use. > > > > > > Why isn't it "ready for use" then? > > > A complete mt7621-pll driver will contain two parts: > > > 1. A clock provider which outputs several clocks > > > 2. A clock gate with parent clocks properly configured > > > > > > Two clocks provided here are just two clocks that can't be controlled > > > in kernel no matter where it goes (arch/mips/ralink or drivers/clk). > > > Having a working CPU clock provider is better than defining a fixed > > > clock in dts because CPU clock can be controlled by bootloader. > > > (BTW description for CPU PLL register is also missing in datasheet.) > > > Clock gate is an unrelated part and there is no information to > > > properly implement it unless MTK decided to release a clock plan > > > somehow. > > > > With other words, your complete system is running with unknown clock > > rates. > > And without this patchset the complete system is running with unknown > clock and, even worse, we make assumptions about what clock bootloader > uses, hardcoded it in dts and hope it is the correct value. > > > The source clock in the clock three can be configured differently > > by bootloader but you don't know how it is done how and it is not > > documented. > > Actually, I don't know about this and I didn't wrote the original > clock calculation code. I just ported it from downstream OpenWrt > kernel. Here's a piece of code from Mediatek's SDK kernel: > > case 0: > reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44)); > cpu_fdiv = ((reg >> 8) & 0x1F); > cpu_ffrac = (reg & 0x1F); > mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000; > break; > case 1: //CPU PLL > reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648)); > fbdiv = ((reg >> 4) & 0x7F) + 1; > reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10)); > reg = (reg >> 6) & 0x7; > if(reg >= 6) { //25Mhz Xtal > mips_cpu_feq = 25 * fbdiv * 1000 * 1000; > } else if(reg >=3) { //40Mhz Xtal > mips_cpu_feq = 20 * fbdiv * 1000 * 1000; > } else { // 20Mhz Xtal > /* TODO */ > } > break; > > > > > > > >> This code is currently on prototyping phase > > > > > > Code for clock calculation is done, not "prototyping". > > > > > >> It means, we cannot expect that this driver will be fixed any time soon. > > > > > > I think clock gating is a separated feature instead of a broken part > > > that has to be fixed. > > > > Ok, i would agree with it. But from what you said, this feature will be > > never implemented. > > > > So, I repeat my question. What is the point to upstream code for a > > system, which has not enough information to get proper clock rate even > > for uart? or is uart running with cpu or bus clock rate? > > uart runs of a fixed 50MHz clock according to another piece of code > from MTK SDK: > (a pastebin version here for better readability. This specific > question has nothing to do with patch reviewing and doesn't need to be > preserved in mail forever.) > https://paste.ubuntu.com/p/fYmtDFW9nh/ > > I could ask the same question: > What is the point of upstreaming an incomplete MT7621 support in the > first place? Current MT7621 support in upstream kernel works only for > mt7621a not mt7621s and it runs of unknown clocks. These kind of code > should stay in downstream projects like OpenWrt forever isn't it? And in fact you've upstreamed a broken ag71xx driver anyway. This debate goes nowhere. I've clarified the situation and made my point. Of course I can't read through the ancient and heavily hacked vendor kernel to figure out a clock plan myself. Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()
- Ursprüngliche Mail - > changelog from v2: > - transform EIO to EFSCORRUPTED as suggested by Matthew; erofs does not define EFSCORRUPTED, so the build fails. At least on Linus' tree as of today. Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel wrote: > > Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > > Hi! > > > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel > > wrote: > >> > We have at least 2 know registers: > SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped > refclock. PLL and dividers used for CPU and some sort of BUS (AHB?). > SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for > all or some ip cores. > What is probably missing is a set of dividers for > each ip core. From your words it is not document. > >>> > >>> The specific missing part I was referring to, is parent clocks for > >>> every gates. I'm not going to assume this with current openwrt device > >>> tree because some peripherals doesn't have a clock binding at all or > >>> have a dummy one there. > >> > >> Ok, then I do not understand what is the motivation to upstream > >> something what is not nearly ready for use. > > > > Why isn't it "ready for use" then? > > A complete mt7621-pll driver will contain two parts: > > 1. A clock provider which outputs several clocks > > 2. A clock gate with parent clocks properly configured > > > > Two clocks provided here are just two clocks that can't be controlled > > in kernel no matter where it goes (arch/mips/ralink or drivers/clk). > > Having a working CPU clock provider is better than defining a fixed > > clock in dts because CPU clock can be controlled by bootloader. > > (BTW description for CPU PLL register is also missing in datasheet.) > > Clock gate is an unrelated part and there is no information to > > properly implement it unless MTK decided to release a clock plan > > somehow. > > With other words, your complete system is running with unknown clock > rates. And without this patchset the complete system is running with unknown clock and, even worse, we make assumptions about what clock bootloader uses, hardcoded it in dts and hope it is the correct value. > The source clock in the clock three can be configured differently > by bootloader but you don't know how it is done how and it is not > documented. Actually, I don't know about this and I didn't wrote the original clock calculation code. I just ported it from downstream OpenWrt kernel. Here's a piece of code from Mediatek's SDK kernel: case 0: reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44)); cpu_fdiv = ((reg >> 8) & 0x1F); cpu_ffrac = (reg & 0x1F); mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000; break; case 1: //CPU PLL reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648)); fbdiv = ((reg >> 4) & 0x7F) + 1; reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10)); reg = (reg >> 6) & 0x7; if(reg >= 6) { //25Mhz Xtal mips_cpu_feq = 25 * fbdiv * 1000 * 1000; } else if(reg >=3) { //40Mhz Xtal mips_cpu_feq = 20 * fbdiv * 1000 * 1000; } else { // 20Mhz Xtal /* TODO */ } break; > > >> This code is currently on prototyping phase > > > > Code for clock calculation is done, not "prototyping". > > > >> It means, we cannot expect that this driver will be fixed any time soon. > > > > I think clock gating is a separated feature instead of a broken part > > that has to be fixed. > > Ok, i would agree with it. But from what you said, this feature will be > never implemented. > > So, I repeat my question. What is the point to upstream code for a > system, which has not enough information to get proper clock rate even > for uart? or is uart running with cpu or bus clock rate? uart runs of a fixed 50MHz clock according to another piece of code from MTK SDK: (a pastebin version here for better readability. This specific question has nothing to do with patch reviewing and doesn't need to be preserved in mail forever.) https://paste.ubuntu.com/p/fYmtDFW9nh/ I could ask the same question: What is the point of upstreaming an incomplete MT7621 support in the first place? Current MT7621 support in upstream kernel works only for mt7621a not mt7621s and it runs of unknown clocks. These kind of code should stay in downstream projects like OpenWrt forever isn't it? Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: move erofs out of staging
- Ursprüngliche Mail - >> While digging a little into the code I noticed that you have very few >> checks of the on-disk data. >> For example ->u.i_blkaddr. I gave it a try and created a >> malformed filesystem where u.i_blkaddr is 0xdeadbeef, it causes the kernel >> to loop forever around erofs_read_raw_page(). > > I don't fuzz all the on-disk fields for EROFS, I will do later.. > You can see many in-kernel filesystems are still hardening the related > stuff. Anyway, I will dig into this field you mentioned recently, but > I think it can be fixed easily later. This is no excuse to redo all these bugs. :-) I know that many in-kernel filesystems trust the disk ultimately, this is a problem and huge attack vector. Thanks, //richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > Hi! > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel wrote: >> We have at least 2 know registers: SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped refclock. PLL and dividers used for CPU and some sort of BUS (AHB?). SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for all or some ip cores. What is probably missing is a set of dividers for each ip core. From your words it is not document. >>> >>> The specific missing part I was referring to, is parent clocks for >>> every gates. I'm not going to assume this with current openwrt device >>> tree because some peripherals doesn't have a clock binding at all or >>> have a dummy one there. >> >> Ok, then I do not understand what is the motivation to upstream >> something what is not nearly ready for use. > > Why isn't it "ready for use" then? > A complete mt7621-pll driver will contain two parts: > 1. A clock provider which outputs several clocks > 2. A clock gate with parent clocks properly configured > > Two clocks provided here are just two clocks that can't be controlled > in kernel no matter where it goes (arch/mips/ralink or drivers/clk). > Having a working CPU clock provider is better than defining a fixed > clock in dts because CPU clock can be controlled by bootloader. > (BTW description for CPU PLL register is also missing in datasheet.) > Clock gate is an unrelated part and there is no information to > properly implement it unless MTK decided to release a clock plan > somehow. With other words, your complete system is running with unknown clock rates. The source clock in the clock three can be configured differently by bootloader but you don't know how it is done how and it is not documented. >> This code is currently on prototyping phase > > Code for clock calculation is done, not "prototyping". > >> It means, we cannot expect that this driver will be fixed any time soon. > > I think clock gating is a separated feature instead of a broken part > that has to be fixed. Ok, i would agree with it. But from what you said, this feature will be never implemented. So, I repeat my question. What is the point to upstream code for a system, which has not enough information to get proper clock rate even for uart? or is uart running with cpu or bus clock rate? -- Regards, Oleksij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel wrote: > > >> We have at least 2 know registers: > >> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped > >> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?). > >> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for > >> all or some ip cores. > >> What is probably missing is a set of dividers for > >> each ip core. From your words it is not document. > > > > The specific missing part I was referring to, is parent clocks for > > every gates. I'm not going to assume this with current openwrt device > > tree because some peripherals doesn't have a clock binding at all or > > have a dummy one there. > > Ok, then I do not understand what is the motivation to upstream > something what is not nearly ready for use. Why isn't it "ready for use" then? A complete mt7621-pll driver will contain two parts: 1. A clock provider which outputs several clocks 2. A clock gate with parent clocks properly configured Two clocks provided here are just two clocks that can't be controlled in kernel no matter where it goes (arch/mips/ralink or drivers/clk). Having a working CPU clock provider is better than defining a fixed clock in dts because CPU clock can be controlled by bootloader. (BTW description for CPU PLL register is also missing in datasheet.) Clock gate is an unrelated part and there is no information to properly implement it unless MTK decided to release a clock plan somehow. > This code is currently on prototyping phase Code for clock calculation is done, not "prototyping". > It means, we cannot expect that this driver will be fixed any time soon. I think clock gating is a separated feature instead of a broken part that has to be fixed. Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: Remove ion device tree bindings from the TODO
On Sun, Aug 18, 2019 at 01:47:38AM -0400, Donald Yandt wrote: > On Sun, Aug 18, 2019 at 1:03 AM Greg KH wrote: > > > > On Sat, Aug 17, 2019 at 05:37:58PM -0400, Donald Yandt wrote: > > > This patch removes the todo for the ion chunk and > > > carveout device tree bindings. > > > > > > Signed-off-by: Donald Yandt > > > --- > > > drivers/staging/android/TODO | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO > > > index fbf015cc6..767dd98fd 100644 > > > --- a/drivers/staging/android/TODO > > > +++ b/drivers/staging/android/TODO > > > @@ -6,8 +6,6 @@ TODO: > > > > > > > > > ion/ > > > - - Add dt-bindings for remaining heaps (chunk and carveout heaps). This > > > would > > > - involve putting appropriate bindings in a memory node for Ion to find. > > > - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0) > > > - Better test framework (integration with VGEM was suggested) > > > > > > > This is already done? Do you have a pointer to the git commit id(s) > > that did it? > > > > thanks, > > > > greg k-h > > Hi Greg, > > Both the chunk and carveout heaps were removed from ion, > so unless I'm mistaken there's no need to implement the device tree > bindings for them. > > Commits that removed both heaps: > - 23a4388f2 staging: android: ion: Remove file ion_chunk_heap.c > - eadbf7a34 staging: android: ion: Remove file ion_carveout_heap.c Great, can you resend this patch with that information in the changelog? As you wrote it, it could be implied that these tasks were actually completed :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 18.08.19 um 04:29 schrieb Chuanhong Guo: > Hi! > > On Sun, Aug 18, 2019 at 2:06 AM Oleksij Rempel wrote: SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks. Jist wild assumption. All peripheral devices are suing bus clock. >>> >>> This assumption is incorrect. When this patchset is applied in >>> OpenWrt, I asked the author why there's still a fixed clock in >>> mt7621.dtsi, He told me that there's another clock for those unchanged >>> peripherals and he doesn't have time to write a clock provider for it. >> >> Can you please provide a link to this patch or email. > > This discussion is in Chinese and using an IM software so there's no > link available. > >> We have at least 2 know registers: >> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped >> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?). >> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for >> all or some ip cores. >> What is probably missing is a set of dividers for >> each ip core. From your words it is not document. > > The specific missing part I was referring to, is parent clocks for > every gates. I'm not going to assume this with current openwrt device > tree because some peripherals doesn't have a clock binding at all or > have a dummy one there. Ok, then I do not understand what is the motivation to upstream something what is not nearly ready for use. This code is currently on prototyping phase and you have no information how to make it ready. It means, we cannot expect that this driver will be fixed any time soon. -- Regards, Oleksij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel