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

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

Herbert Xu <herbert@gondor.apana.org.au> writes:

> > +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	
> 
> This is a good catch.  But you probably need to fix the vif stuff as
> well.

Yes, right. I didn't notice it. Thanks.

   - use seq_release_private for ipmr_vif/mfc_fops.

The anothor bug, it->cache need in *->seq_start* by NULL. Otherwise,
it->cache can be pointed the previous state by lseek().

e.g.
    ->seq_next()
         it->cache = &mfc_unres_queue
    ->seq_stop()
    lseek()
        ->seq_start()
              return SEQ_START_TOKEN (it->cache still has &mfc_unres_queue)
        ->seq_show()
        ->seq_stop()
              bang

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>




---

 net/ipv4/ipmr.c |   45 ++++++++++++++++++++-------------------------
 1 files changed, 20 insertions(+), 25 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 16:53:05.000000000 +0900
@@ -1702,7 +1702,7 @@ static struct file_operations ipmr_vif_f
 	.open    = ipmr_vif_open,
 	.read    = seq_read,
 	.llseek  = seq_lseek,
-	.release = seq_release,
+	.release = seq_release_private,
 };
 
 struct ipmr_mfc_iter {
@@ -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,9 +1733,11 @@ static struct mfc_cache *ipmr_mfc_seq_id
 	return NULL;
 }
 
-
 static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
 {
+	struct ipmr_mfc_iter *it = seq->private;
+	it->cache = NULL;
+	it->ct = 0;
 	return *pos ? ipmr_mfc_seq_idx(seq->private, *pos - 1) 
 		: SEQ_START_TOKEN;
 }
@@ -1754,31 +1755,27 @@ 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];
+		it->cache = &mfc_unres_queue;
+		it->ct = 0;
+		spin_lock_bh(&mfc_unres_lock);
+		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;
 }
 
@@ -1846,7 +1843,6 @@ static int ipmr_mfc_open(struct inode *i
 	if (rc)
 		goto out_kfree;
 
-	memset(s, 0, sizeof(*s));
 	seq = file->private_data;
 	seq->private = s;
 out:
@@ -1854,7 +1850,6 @@ out:
 out_kfree:
 	kfree(s);
 	goto out;
-
 }
 
 static struct file_operations ipmr_mfc_fops = {
@@ -1862,7 +1857,7 @@ static struct file_operations ipmr_mfc_f
 	.open    = ipmr_mfc_open,
 	.read    = seq_read,
 	.llseek  = seq_lseek,
-	.release = seq_release,
+	.release = seq_release_private,
 };
 #endif	
 

_