* [PATCH] erofs: initialized fields can only be observed after bit is set [not found] <20210209130618.15838-1-hsiangkao.ref@aol.com> @ 2021-02-09 13:06 ` Gao Xiang 2021-02-10 12:09 ` Chao Yu 2021-02-11 3:45 ` Chao Yu 0 siblings, 2 replies; 4+ messages in thread From: Gao Xiang @ 2021-02-09 13:06 UTC (permalink / raw) To: linux-erofs; +Cc: Chao Yu, LKML, Gao Xiang, stable, Huang Jianan From: Gao Xiang <hsiangkao@redhat.com> Currently, although set_bit() & test_bit() pairs are used as a fast- path for initialized configurations. However, these atomic ops are actually relaxed forms. Instead, load-acquire & store-release form is needed to make sure uninitialized fields won't be observed in advance here (yet no such corresponding bitops so use full barriers instead.) Fixes: 62dc45979f3f ("staging: erofs: fix race of initializing xattrs of a inode at the same time") Fixes: 152a333a5895 ("staging: erofs: add compacted compression indexes support") Cc: <stable@vger.kernel.org> # 5.3+ Reported-by: Huang Jianan <huangjianan@oppo.com> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- fs/erofs/xattr.c | 10 +++++++++- fs/erofs/zmap.c | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c index 5bde77d70852..47314a26767a 100644 --- a/fs/erofs/xattr.c +++ b/fs/erofs/xattr.c @@ -48,8 +48,14 @@ static int init_inode_xattrs(struct inode *inode) int ret = 0; /* the most case is that xattrs of this inode are initialized. */ - if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) + if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) { + /* + * paired with smp_mb() at the end of the function to ensure + * fields will only be observed after the bit is set. + */ + smp_mb(); return 0; + } if (wait_on_bit_lock(&vi->flags, EROFS_I_BL_XATTR_BIT, TASK_KILLABLE)) return -ERESTARTSYS; @@ -137,6 +143,8 @@ static int init_inode_xattrs(struct inode *inode) } xattr_iter_end(&it, atomic_map); + /* paired with smp_mb() at the beginning of the function. */ + smp_mb(); set_bit(EROFS_I_EA_INITED_BIT, &vi->flags); out_unlock: diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c index ae325541884e..14d2de35110c 100644 --- a/fs/erofs/zmap.c +++ b/fs/erofs/zmap.c @@ -36,8 +36,14 @@ static int z_erofs_fill_inode_lazy(struct inode *inode) void *kaddr; struct z_erofs_map_header *h; - if (test_bit(EROFS_I_Z_INITED_BIT, &vi->flags)) + if (test_bit(EROFS_I_Z_INITED_BIT, &vi->flags)) { + /* + * paired with smp_mb() at the end of the function to ensure + * fields will only be observed after the bit is set. + */ + smp_mb(); return 0; + } if (wait_on_bit_lock(&vi->flags, EROFS_I_BL_Z_BIT, TASK_KILLABLE)) return -ERESTARTSYS; @@ -83,6 +89,8 @@ static int z_erofs_fill_inode_lazy(struct inode *inode) vi->z_physical_clusterbits[1] = vi->z_logical_clusterbits + ((h->h_clusterbits >> 5) & 7); + /* paired with smp_mb() at the beginning of the function */ + smp_mb(); set_bit(EROFS_I_Z_INITED_BIT, &vi->flags); unmap_done: kunmap_atomic(kaddr); -- 2.24.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] erofs: initialized fields can only be observed after bit is set 2021-02-09 13:06 ` [PATCH] erofs: initialized fields can only be observed after bit is set Gao Xiang @ 2021-02-10 12:09 ` Chao Yu 2021-02-10 12:38 ` Gao Xiang 2021-02-11 3:45 ` Chao Yu 1 sibling, 1 reply; 4+ messages in thread From: Chao Yu @ 2021-02-10 12:09 UTC (permalink / raw) To: Gao Xiang, linux-erofs; +Cc: LKML, stable Hi Xiang, On 2021/2/9 21:06, Gao Xiang via Linux-erofs wrote: > From: Gao Xiang <hsiangkao@redhat.com> > > Currently, although set_bit() & test_bit() pairs are used as a fast- > path for initialized configurations. However, these atomic ops are > actually relaxed forms. Instead, load-acquire & store-release form is > needed to make sure uninitialized fields won't be observed in advance > here (yet no such corresponding bitops so use full barriers instead.) > > Fixes: 62dc45979f3f ("staging: erofs: fix race of initializing xattrs of a inode at the same time") > Fixes: 152a333a5895 ("staging: erofs: add compacted compression indexes support") > Cc: <stable@vger.kernel.org> # 5.3+ > Reported-by: Huang Jianan <huangjianan@oppo.com> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > fs/erofs/xattr.c | 10 +++++++++- > fs/erofs/zmap.c | 10 +++++++++- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c > index 5bde77d70852..47314a26767a 100644 > --- a/fs/erofs/xattr.c > +++ b/fs/erofs/xattr.c > @@ -48,8 +48,14 @@ static int init_inode_xattrs(struct inode *inode) > int ret = 0; > > /* the most case is that xattrs of this inode are initialized. */ > - if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) > + if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) { > + /* > + * paired with smp_mb() at the end of the function to ensure > + * fields will only be observed after the bit is set. > + */ > + smp_mb(); I can understand below usage, since w/o smp_mb(), xattr initialization could be done after set_bit(EROFS_I_EA_INITED_BIT), then other apps could see out-of-update xattr info after that bit check. So what out-of-order execution do we need to avoid by adding above barrier? Thanks, > + /* paired with smp_mb() at the beginning of the function. */ > + smp_mb(); > set_bit(EROFS_I_EA_INITED_BIT, &vi->flags); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] erofs: initialized fields can only be observed after bit is set 2021-02-10 12:09 ` Chao Yu @ 2021-02-10 12:38 ` Gao Xiang 0 siblings, 0 replies; 4+ messages in thread From: Gao Xiang @ 2021-02-10 12:38 UTC (permalink / raw) To: Chao Yu; +Cc: Gao Xiang, linux-erofs, LKML, stable Hi Chao, On Wed, Feb 10, 2021 at 08:09:22PM +0800, Chao Yu wrote: > Hi Xiang, > > On 2021/2/9 21:06, Gao Xiang via Linux-erofs wrote: > > From: Gao Xiang <hsiangkao@redhat.com> > > > > Currently, although set_bit() & test_bit() pairs are used as a fast- > > path for initialized configurations. However, these atomic ops are > > actually relaxed forms. Instead, load-acquire & store-release form is > > needed to make sure uninitialized fields won't be observed in advance > > here (yet no such corresponding bitops so use full barriers instead.) > > > > Fixes: 62dc45979f3f ("staging: erofs: fix race of initializing xattrs of a inode at the same time") > > Fixes: 152a333a5895 ("staging: erofs: add compacted compression indexes support") > > Cc: <stable@vger.kernel.org> # 5.3+ > > Reported-by: Huang Jianan <huangjianan@oppo.com> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > fs/erofs/xattr.c | 10 +++++++++- > > fs/erofs/zmap.c | 10 +++++++++- > > 2 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c > > index 5bde77d70852..47314a26767a 100644 > > --- a/fs/erofs/xattr.c > > +++ b/fs/erofs/xattr.c > > @@ -48,8 +48,14 @@ static int init_inode_xattrs(struct inode *inode) > > int ret = 0; > > /* the most case is that xattrs of this inode are initialized. */ > > - if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) > > + if (test_bit(EROFS_I_EA_INITED_BIT, &vi->flags)) { > > + /* > > + * paired with smp_mb() at the end of the function to ensure > > + * fields will only be observed after the bit is set. > > + */ > > + smp_mb(); > > I can understand below usage, since w/o smp_mb(), xattr initialization > could be done after set_bit(EROFS_I_EA_INITED_BIT), then other apps could > see out-of-update xattr info after that bit check. > > So what out-of-order execution do we need to avoid by adding above barrier? > These is one-shot lazy initialization to delay read/parse xattr/compress indexes to the first read since many workloads don't need such information at all. Without such memory barrier pairs, if two (or more) initializations runs nearly simultaneously, the paralleled process could observe uninitialized values (zeroed values). That is OPPO colleagues found on their products. Yeah, this could be somewhat kind of out-of-order, yet more specifically called memory reordering. Xattr/compress indexes initialization could be lazily observed by the CPU after it observed that EROFS_I_EA_INITED_BIT/ EROFS_I_Z_INITED_BIT is set. So we need memory barrier pairs to guarantee such data ordering. Thanks, Gao Xiang > Thanks, > > > + /* paired with smp_mb() at the beginning of the function. */ > > + smp_mb(); > > set_bit(EROFS_I_EA_INITED_BIT, &vi->flags); > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] erofs: initialized fields can only be observed after bit is set 2021-02-09 13:06 ` [PATCH] erofs: initialized fields can only be observed after bit is set Gao Xiang 2021-02-10 12:09 ` Chao Yu @ 2021-02-11 3:45 ` Chao Yu 1 sibling, 0 replies; 4+ messages in thread From: Chao Yu @ 2021-02-11 3:45 UTC (permalink / raw) To: Gao Xiang, linux-erofs; +Cc: LKML, stable On 2021/2/9 21:06, Gao Xiang via Linux-erofs wrote: > From: Gao Xiang <hsiangkao@redhat.com> > > Currently, although set_bit() & test_bit() pairs are used as a fast- > path for initialized configurations. However, these atomic ops are > actually relaxed forms. Instead, load-acquire & store-release form is > needed to make sure uninitialized fields won't be observed in advance > here (yet no such corresponding bitops so use full barriers instead.) > > Fixes: 62dc45979f3f ("staging: erofs: fix race of initializing xattrs of a inode at the same time") > Fixes: 152a333a5895 ("staging: erofs: add compacted compression indexes support") > Cc: <stable@vger.kernel.org> # 5.3+ > Reported-by: Huang Jianan <huangjianan@oppo.com> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> Thanks for detailed explanation for barrier offline. Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-11 3:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210209130618.15838-1-hsiangkao.ref@aol.com> 2021-02-09 13:06 ` [PATCH] erofs: initialized fields can only be observed after bit is set Gao Xiang 2021-02-10 12:09 ` Chao Yu 2021-02-10 12:38 ` Gao Xiang 2021-02-11 3:45 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).