Re: [BUGS] [CHECKER] 99 synchronization bugs and a lock summary database

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Stephen Hemminger <shemminger@osdl.org> writes:

> > > Andrew Morton <akpm@osdl.org> wrote:
> > > > 
> > > > Could someone please take a look at the locking in
> > > > net/ipv4/ipmr.c:ipmr_mfc_seq_next?  It seems rather broken.
> > > 
> > > Obfuscated yes, broken no.
> > 
> > Really?  Even if that goto
> > 
> > 	if (it->cache == &mfc_unres_queue) 
> > 		goto end_of_list;
> > 
> > is taken?
> 
> The problem is that the seq_file interface is trying to create an iterator
> over  a set of data.  The interface expects that the usage will always fit
> a given pattern.
> 
> The implied semantic is:
> 	if returned handle is NULL, then nothing is locked and it->cache = NULL
> 
> 	if returned handle is in the mfc_cache_table then
> 		it->cache points to mfc_cache_array and mrt_lock is held
> 	if returned handle is in the mfc_unres_queue then
> 		it->cache points to mfc_unres_queue and mfc_unres_lock is held
> 
> the seq_next is only valid to mean give me the next entry after the passed handle
> therefore if there are no more entries after the handle and the handle was on
> the unresolved queue, then the end is reached. 
> 
> When seq_stop is called, the it->cache pointer will be NULL so no unlock is done.

How about the following patch? At lease, this seems to need ipmr_mfc_release().
Untested patch, sorry.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


 net/ipv4/ipmr.c |   50 ++++++++++++++++++++++++++------------------------
 1 files changed, 26 insertions(+), 24 deletions(-)

diff -puN net/ipv4/ipmr.c~ipmr-cleanup net/ipv4/ipmr.c
--- linux-2.6.7/net/ipv4/ipmr.c~ipmr-cleanup	2004-07-03 14:13:50.000000000 +0900
+++ linux-2.6.7-hirofumi/net/ipv4/ipmr.c	2004-07-03 14:30:54.000000000 +0900
@@ -1710,7 +1710,6 @@ struct ipmr_mfc_iter {
 	int ct;
 };
 
-
 static struct mfc_cache *ipmr_mfc_seq_idx(struct ipmr_mfc_iter *it, loff_t pos)
 {
 	struct mfc_cache *mfc;
@@ -1734,7 +1733,6 @@ static struct mfc_cache *ipmr_mfc_seq_id
 	return NULL;
 }
 
-
 static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	return *pos ? ipmr_mfc_seq_idx(seq->private, *pos - 1) 
@@ -1754,31 +1752,29 @@ static void *ipmr_mfc_seq_next(struct se
 	if (mfc->next)
 		return mfc->next;
 	
-	if (it->cache == &mfc_unres_queue) 
-		goto end_of_list;
+	if (it->cache == mfc_cache_array) {
+		while (++it->ct < MFC_LINES) {
+			mfc = mfc_cache_array[it->ct];
+			if (mfc)
+				return mfc;
+		}
 
-	BUG_ON(it->cache != mfc_cache_array);
+		/*
+		 * exhausted cache_array, show unresolved.
+		 * So switch to mfc_unres_queue.
+		 */
+		read_unlock(&mrt_lock);
 
-	while (++it->ct < MFC_LINES) {
-		mfc = mfc_cache_array[it->ct];
-		if (mfc)
-			return mfc;
+		it->cache = &mfc_unres_queue;
+		it->ct = 0;
+		spin_lock_bh(&mfc_unres_lock);
+		if (it->cache == mfc_unres_queue) {
+			mfc = mfc_unres_queue;
+			if (mfc)
+				return mfc;
+		}
 	}
 
-	/* exhausted cache_array, show unresolved */
-	read_unlock(&mrt_lock);
-	it->cache = &mfc_unres_queue;
-	it->ct = 0;
-		
-	spin_lock_bh(&mfc_unres_lock);
-	mfc = mfc_unres_queue;
-	if (mfc) 
-		return mfc;
-
- end_of_list:
-	spin_unlock_bh(&mfc_unres_lock);
-	it->cache = NULL;
-
 	return NULL;
 }
 
@@ -1854,7 +1850,13 @@ out:
 out_kfree:
 	kfree(s);
 	goto out;
+}
 
+static int ipmr_mfc_release(struct inode *inode, struct file *file)
+{
+	struct seq_file *seq = file->private_data;
+	kfree(seq->private);
+	return seq_release(inode, file);
 }
 
 static struct file_operations ipmr_mfc_fops = {
@@ -1862,7 +1864,7 @@ static struct file_operations ipmr_mfc_f
 	.open    = ipmr_mfc_open,
 	.read    = seq_read,
 	.llseek  = seq_lseek,
-	.release = seq_release,
+	.release = ipmr_mfc_release,
 };
 #endif	
 

_