lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 14 May 2022 18:34:35 +0100 From: Matthew Wilcox <willy@...radead.org> To: Vasily Averin <vvs@...nvz.org> Cc: Dan Williams <dan.j.williams@...el.com>, Jan Kara <jack@...e.cz>, Alexander Viro <viro@...iv.linux.org.uk>, kernel@...nvz.org, linux-kernel@...r.kernel.org, nvdimm@...ts.linux.dev, linux-fsdevel@...r.kernel.org Subject: Re: [PATCH] sparse: use force attribute for vm_fault_t casts On Sat, May 14, 2022 at 05:26:21PM +0300, Vasily Averin wrote: > Fixes sparse warnings: > ./include/trace/events/fs_dax.h:10:1: sparse: > got restricted vm_fault_t > ./include/trace/events/fs_dax.h:153:1: sparse: > got restricted vm_fault_t > fs/dax.c:563:39: sparse: got restricted vm_fault_t > fs/dax.c:565:39: sparse: got restricted vm_fault_t > fs/dax.c:569:31: sparse: got restricted vm_fault_t > fs/dax.c:1055:41: sparse: > got restricted vm_fault_t [assigned] [usertype] ret > fs/dax.c:1461:46: sparse: got restricted vm_fault_t [usertype] ret > fs/dax.c:1477:21: sparse: > expected restricted vm_fault_t [assigned] [usertype] ret > fs/dax.c:1518:51: sparse: > got restricted vm_fault_t [assigned] [usertype] ret > fs/dax.c:1599:21: sparse: > expected restricted vm_fault_t [assigned] [usertype] ret > fs/dax.c:1633:62: sparse: > got restricted vm_fault_t [assigned] [usertype] ret > fs/dax.c:1696:55: sparse: got restricted vm_fault_t > fs/dax.c:1711:58: sparse: > got restricted vm_fault_t [assigned] [usertype] ret > > vm_fault_t type is bitwise and requires __force attribute for any casts. Well, this patch is all kinds of messy. I would rather we had better abstractions. For example ... > @@ -560,13 +560,13 @@ static void *grab_mapping_entry(struct xa_state *xas, > if (xas_nomem(xas, mapping_gfp_mask(mapping) & ~__GFP_HIGHMEM)) > goto retry; > if (xas->xa_node == XA_ERROR(-ENOMEM)) > - return xa_mk_internal(VM_FAULT_OOM); > + return xa_mk_internal((__force unsigned long)VM_FAULT_OOM); > if (xas_error(xas)) > - return xa_mk_internal(VM_FAULT_SIGBUS); > + return xa_mk_internal((__force unsigned long)VM_FAULT_SIGBUS); > return entry; > fallback: > xas_unlock_irq(xas); > - return xa_mk_internal(VM_FAULT_FALLBACK); > + return xa_mk_internal((__force unsigned long)VM_FAULT_FALLBACK); > } return vm_fault_encode(VM_FAULT_xxx); > /** > @@ -1052,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas, > DAX_ZERO_PAGE, false); > > ret = vmf_insert_mixed(vmf->vma, vaddr, pfn); > - trace_dax_load_hole(inode, vmf, ret); > + trace_dax_load_hole(inode, vmf, (__force int)ret); Seems like trace_dax_load_hole() should take a vm_fault_t? > - trace_dax_pte_fault(iter.inode, vmf, ret); > + trace_dax_pte_fault(iter.inode, vmf, (__force int)ret); Ditto. > @@ -1474,7 +1474,7 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp, > > entry = grab_mapping_entry(&xas, mapping, 0); > if (xa_is_internal(entry)) { > - ret = xa_to_internal(entry); > + ret = (__force vm_fault_t)xa_to_internal(entry); vm_fault_decode(entry)? ... the others seem like more of the same. So I'm in favour of what you're doing, but would rather it were done differently. Generally seeing __force casts in the body of a function is a sign that things are wrong; it's better to have them hidden in abstractions.
Powered by blists - more mailing lists