Ocfs2 devel - Jan 2020 - [PATCH 1/4] lockres_seq_next should increase position index

On 20/1/23 15:39, Vasily Averin wrote:
> if seq_file .next fuction does not change position index,
> read after some lseek can generate unexpected output.
> 
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.kernel.org_show-5Fbug.cgi-3Fid-3D206283&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=C7gAd4uDxlAvTdc0vmU6X8CMk6L2iDY8-HD0qT6Fo7Y&m=lFWWo4w3KGQSd5EANoUFCp9aybqo1xeFs5MD0dgTnkA&s=dFZ-8He72Ffi8NGZB7iFik1nYOMs9wfzjDFpATCr72I&e=
> Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
We'd better add the missing tag:

Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
interface")
> ---
>  fs/ocfs2/dlm/dlmdebug.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index 4d0b452..08a19c9 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -590,6 +590,7 @@ static void lockres_seq_stop(struct seq_file *m, void
*v)
>  
>  static void *lockres_seq_next(struct seq_file *m, void *v, loff_t *pos)
>  {
> +	(*pos)++;
>  	return NULL;
>  }
>  
>

v2: resend with improved patch description

In Aug 2018 NeilBrown noticed 
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
interface")
"Some ->next functions do not increment *pos when they return NULL...
Note that such ->next functions are buggy and should be fixed. 
A simple demonstration is
   
dd if=/proc/swaps bs=1000 skip=1
    
Choose any block size larger than the size of /proc/swaps.  This will
always show the whole last line of /proc/swaps"

/proc/swaps output was fixed recently, however there are lot of other
affected files and 4 of them are related to ocfs2.

Unfortunately I'm not familiar with ocfs2 and cannot verify the patches
locally.

Usually you can observe following related problems:
- read after lseek beyond end of file, described above by NeilBrown
 "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly
generate whole last line

- read after lseek on into middle of last line will output expected rest of
 last line but then repeat whole last line once again. 

- If .show() function generates multi-line output following bash script will
never finish.

 $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE

Link:
https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!Jq0fspl3uRa_Xiu-ifjc60ajewi308CVdv8Dq-9kHzOjkRYAd1YKlDU1a1TQdmMTBPWO8g$

Vasily Averin (4):
  lockres_seq_next should increase position index
  ocfs2_dlm_seq_next should increase position index
  nst_seq_next should increase position index
  sc_seq_next should increase position index

 fs/ocfs2/cluster/netdebug.c | 2 ++
 fs/ocfs2/dlm/dlmdebug.c     | 1 +
 fs/ocfs2/dlmglue.c          | 1 +
 3 files changed, 4 insertions(+)

-- 
1.8.3.1

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
...")
Link:
https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!L2HMxpDQh7db9dQNbu_Tp0uSxEzRVFvZ0bxczbKOsvt3hiKOnWevYToiSclR_wkhhve7qA$
Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
---
 fs/ocfs2/dlm/dlmdebug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index c5c6efb..3e395cd 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -590,6 +590,7 @@ static void lockres_seq_stop(struct seq_file *m, void *v)
 
 static void *lockres_seq_next(struct seq_file *m, void *v, loff_t *pos)
 {
+	(*pos)++;
 	return NULL;
 }
 
-- 
1.8.3.1

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
...")
Link:
https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!MSixDxrphasW57tPl4VD6FKe6ZxcdQkP0oN-cWbZFdRxlNR1AOaFgm3-OunznZSXFEf0Nw$
Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
---
 fs/ocfs2/dlmglue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index cb9e6a7..170eb75 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -3080,6 +3080,7 @@ static void *ocfs2_dlm_seq_next(struct seq_file *m, void
*v, loff_t *pos)
 	struct ocfs2_lock_res *iter = v;
 	struct ocfs2_lock_res *dummy = &priv->p_iter_res;
 
+	(*pos)++;
 	spin_lock(&ocfs2_dlm_tracking_lock);
 	iter = ocfs2_dlm_next_res(iter, priv);
 	list_del_init(&dummy->l_debug_list);
-- 
1.8.3.1

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
...")
Link:
https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!PE9FFsjnlLuJDzt36R28ZRQGJ-jhWo4rlejw_3MAX2zd1KvSlIomyKIzzBvc5psf88EhGw$
Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
---
 fs/ocfs2/cluster/netdebug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/cluster/netdebug.c b/fs/ocfs2/cluster/netdebug.c
index 02bf4a1..26fc6a2 100644
--- a/fs/ocfs2/cluster/netdebug.c
+++ b/fs/ocfs2/cluster/netdebug.c
@@ -97,6 +97,7 @@ static void *nst_seq_next(struct seq_file *seq, void *v,
loff_t *pos)
 {
 	struct o2net_send_tracking *nst, *dummy_nst = seq->private;
 
+	++(*pos);
 	spin_lock(&o2net_debug_lock);
 	nst = next_nst(dummy_nst);
 	list_del_init(&dummy_nst->st_net_debug_item);
-- 
1.8.3.1

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
...")
Link:
https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!OngcjY3Xm--sW3K-NHZ9tpRCS9bSp-bm45QwSwULt5bj3vAbmwAs1_LXYQm9uQ3rK7wmDA$
Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
---
 fs/ocfs2/cluster/netdebug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/cluster/netdebug.c b/fs/ocfs2/cluster/netdebug.c
index 26fc6a2..71ddd65 100644
--- a/fs/ocfs2/cluster/netdebug.c
+++ b/fs/ocfs2/cluster/netdebug.c
@@ -251,6 +251,7 @@ static void *sc_seq_next(struct seq_file *seq, void *v,
loff_t *pos)
 	struct o2net_sock_debug *sd = seq->private;
 	struct o2net_sock_container *sc, *dummy_sc = sd->dbg_sock;
 
+	++(*pos);
 	spin_lock(&o2net_debug_lock);
 	sc = next_sc(dummy_sc);
 	list_del_init(&dummy_sc->sc_net_debug_item);
-- 
1.8.3.1

Looks good.
Jun, could you please help verify these changes and give your tested-by? 
Since I don't have ocfs2 cluster locally...

Thanks,
Joseph

On 2020/2/26 14:52, Vasily Averin wrote:
> v2: resend with improved patch description
> 
> In Aug 2018 NeilBrown noticed 
> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
and interface")
> "Some ->next functions do not increment *pos when they return
NULL...
> Note that such ->next functions are buggy and should be fixed. 
> A simple demonstration is
>    
> dd if=/proc/swaps bs=1000 skip=1
>     
> Choose any block size larger than the size of /proc/swaps.  This will
> always show the whole last line of /proc/swaps"
> 
> /proc/swaps output was fixed recently, however there are lot of other
> affected files and 4 of them are related to ocfs2.
> 
> Unfortunately I'm not familiar with ocfs2 and cannot verify the patches
locally.
> 
> Usually you can observe following related problems:
> - read after lseek beyond end of file, described above by NeilBrown
>  "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly
generate whole last line
> 
> - read after lseek on into middle of last line will output expected rest of
>  last line but then repeat whole last line once again. 
> 
> - If .show() function generates multi-line output following bash script
will never finish.
> 
>  $ q=;while read -r r;do echo "$((++q)) $r";done <
AFFECTED_FILE
> 
> Link:
https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!MWBujxdP_p0jZT4u4PHj-NBdhW4f9I2yRxwrFk8cG8MMHecBByM6r53K0M3eU9W5Oe1rdQ$
> 
> Vasily Averin (4):
>   lockres_seq_next should increase position index
>   ocfs2_dlm_seq_next should increase position index
>   nst_seq_next should increase position index
>   sc_seq_next should increase position index
> 
>  fs/ocfs2/cluster/netdebug.c | 2 ++
>  fs/ocfs2/dlm/dlmdebug.c     | 1 +
>  fs/ocfs2/dlmglue.c          | 1 +
>  3 files changed, 4 insertions(+)
>