| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BN6PR01MB2434EB63780B328F89D559DCD8590@BN6PR01MB2434.prod.exchangelabs.com>
Date: Mon, 13 Feb 2017 15:44:15 +0000
From: "Allen-Bond, Michael Rafael" <michael.allen-bond@....edu>
To: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: 14 Bug submissions/code questions
The Bugzilla URLs are all together at the bottom as well.
1. fs/inode.c 804L find_inode_fast structure inode data structure
The data structure is initialized as NULL, is there any guarantee that the head of the list it points to after hlist_for_each_entry() won't be a null value? The assignment seems to assume that the head will always have a struct available to assign to the pointer.
https://bugzilla.kernel.org/show_bug.cgi?id=194251
2. fs/xfs/libxfs/xfs_ialloc.c 1772L xfs_dialloc return/fault return error
There seem to be multiple ambiguous returns in this function. The return values for multiple failed allocation branches return 0, is this handled in the caller function?
https://bugzilla.kernel.org/show_bug.cgi?id=194261
3. net/unix/af_unix.c 2020L unix_stream_sendpage condition should check fast path first
The fast path (L2007) of this function should be called as early as possible. I don't fully understand the semantics of the function, but is this fast path condition dependent on all the conditional branches before it? The branches seem poorly structured regardless (lots of redundant conditions).
https://bugzilla.kernel.org/show_bug.cgi?id=194281
4. tools/perf/util/bpf-prologue.c 140L gen_prologue_fastpath structure args.value is the only arguments used.
The struct probe_trace_arg *args contains multiple values, but only the .value field is used. If that's the only field in *args necessary for this function, is there a better way to pass the values?
https://bugzilla.kernel.org/show_bug.cgi?id=194291
5. mm/slab.c 3155L slab_alloc_node fault ____cache_alloc does not fallback to other nodes
This is addressed already in the code comments, but ____cache_alloc and ____cache_alloc_node should be merged or restructured to allow fallback to other nodes in the fastpath (local cache) allocations.
https://bugzilla.kernel.org/show_bug.cgi?id=194301
6. drivers/block/drbd/drbd_req.c 1432L do_submit condition fast path does not handle read as slow path
****
Is there any guarantee that do_submit doesn't receive read requests? There's a simple check for it in the fast path, but nothing in the slow path, why not implement the check in the beginning of do_submit, since we check for it anyways, and this avoids needlessly calling extra functions.
https://bugzilla.kernel.org/show_bug.cgi?id=194311
7. drivers/scsi/mpt3sas/mpt3sas_base.c 2484L mpt3sas_base_put_smid_fast_path structure descriptor.Default.DescriptorTypeDependent is not used in fast path
Our tool gave us a warning regarding this field being unused, I'm not sure if it bears fixing.
https://bugzilla.kernel.org/show_bug.cgi?id=194321
8. drivers/net/ethernet/qlogic/qed/qed_l2.c 1876L qed_fastpath_stop fault
Is there any guarantee that qed_hw_stop_fastpath won't fail, especially during its sleep range? There's no fault handler or consistency check outside of the function.
https://bugzilla.kernel.org/show_bug.cgi?id=194331
9. drivers/scsi/mpt3sas/mpt3sas_scsih.c 6207L _scsih_ir_fastpath fault
Given the number of error checks and intermittent conditional branches all ending with the same error code, it may be worth restructuring this path to optimistically execute the fast path and then have a single set of error checks at the end, since all of the errors immediately exit the function with a failure code anyways.
https://bugzilla.kernel.org/show_bug.cgi?id=194341
10. drivers/staging/lustre/lustre/lov/lov_io.c 438L lov_io_rw_iter_init condition
This fastpath's value is mitigated by a slowpath that it has to traverse through at the end of the function, and the conditional branches in the slowpath have inconsistent return values (particularly the (unlikely) branch).
https://bugzilla.kernel.org/show_bug.cgi?id=194351
11. drivers/staging/lustre/obdclass/cl_page.c 354L cl_page_find return
The return value in cl_page_alloc can be ERR_PTR(-ENOMEM), which is then returned in cl_page_find. Is this error handled in the caller function? Why is a page used for returning the -ENOMEM error?
https://bugzilla.kernel.org/show_bug.cgi?id=194361
12.drivers/tty/hvc/hvc_console.c 345L hvc_open fault fast path has no way to call hvc_close() when hvc_kick() fails.
The lack of fault handling out of the fast path is based on the assumption that hvc_kick() -->try_to_wake_up() never fails, correct?
https://bugzilla.kernel.org/show_bug.cgi?id=194371
13.
fs: ocfs2: uptodate.c: set_buffer_uptodate(bh) is not clearing if __ocfs2_set_buffer_uptodate fails, which it can.
In ocfs2_set_new_buffer_uptodate, the slow path in ocfs2_set_buffer_uptodate may fail, but ocfs2_set_new_buffer_uptodate will return without calling clear_buffer_uptodate(bh).
Specifically this code snippet from ocfs2_set_new_buffer_uptdate():
{
set_buffer_uptodate(bh);
ocfs2_metadata_cache_io_lock(ci);
ocfs2_set_buffer_uptodate(ci, bh);
ocfs2_metadata_cache_io_unlock(ci);
}
If these two functions are correlated (via <bh>), should they be in the locked portion together? A competing thread may grab the lock, possibly resulting in erroneous values in the buffer.
If set_buffer_uptodate is cleared in another thread, how does that affect ocfs2_set_buffer_uptodate?
https://bugzilla.kernel.org/show_bug.cgi?id=179511
14.
net: unix: af_unix.c unix_stream_sendpage should check fast path first
If possible, the fast path conditions should be checked and entered earlier.
Specifically:
skb = skb_peek_tail(&other->sk_receive_queue);
if (tail && tail == skb) {
skb = newskb;
} else if (!skb || !unix_skb_scm_eq(skb, &scm)) {
if (newskb) {
skb = newskb;
} else {
tail = skb;
goto alloc_skb;
}
--> } else if (newskb) {
/* this is fast path, we don't necessarily need to
* call to kfree_skb even though with newskb == NULL
* this - does no harm
*/
consume_skb(newskb);
newskb = NULL;
}
If this is a fast path, it should be entered sooner, since newskb seems to be independent of the variables in previous checks.
https://bugzilla.kernel.org/show_bug.cgi?id=194281
https://bugzilla.kernel.org/show_bug.cgi?id=194251
https://bugzilla.kernel.org/show_bug.cgi?id=194261
https://bugzilla.kernel.org/show_bug.cgi?id=194281
https://bugzilla.kernel.org/show_bug.cgi?id=194291
https://bugzilla.kernel.org/show_bug.cgi?id=194301
https://bugzilla.kernel.org/show_bug.cgi?id=194311
https://bugzilla.kernel.org/show_bug.cgi?id=194321
https://bugzilla.kernel.org/show_bug.cgi?id=194331
https://bugzilla.kernel.org/show_bug.cgi?id=194341
https://bugzilla.kernel.org/show_bug.cgi?id=194351
https://bugzilla.kernel.org/show_bug.cgi?id=194361
https://bugzilla.kernel.org/show_bug.cgi?id=194371
https://bugzilla.kernel.org/show_bug.cgi?id=179511
https://bugzilla.kernel.org/show_bug.cgi?id=194281
Thank you for your time.
Powered by blists - more mailing lists