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