| Message ID | 1438305123-31675-1-git-send-email-yangds.fnst@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
Am 31.07.2015 um 03:12 schrieb Dongsheng Yang: > This commit make the ubifs_[get|set]xattr protected by ui_mutex. > > Originally, there is a possibility that ubifs_getxattr to get > a wrong value. > > P1 P2 > ---------- ---------- > ubifs_getxattr ubifs_setxattr > - kfree() > - memcpy() > - kmemdup() > > Then ubifs_getxattr() would get a non-sense data. To solve this > problem, this commit make the xattr of ubifs_inode updated in > atomic. so, ui->data needs protection? The comment in fs/ubifs/ubifs.h does not mention ->data. I'm asking because I want to make sure that ui_mutex is the correct lock to take. > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > -v2: > Add more description in commit message > fs/ubifs/xattr.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c > index 96f3448..dec1afd 100644 > --- a/fs/ubifs/xattr.c > +++ b/fs/ubifs/xattr.c > @@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, > if (err) > return err; > > + mutex_lock(&ui->ui_mutex); > kfree(ui->data); > ui->data = kmemdup(value, size, GFP_NOFS); > if (!ui->data) { > @@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, > } > inode->i_size = ui->ui_size = size; > ui->data_len = size; > + mutex_unlock(&ui->ui_mutex); You also need a mutex_unlock(&ui->ui_mutex) under out_free, otherwise the if (!ui->data) { branch will trigger a deadlock. Thanks, //richard
On 08/04/2015 04:27 AM, Richard Weinberger wrote: > Am 31.07.2015 um 03:12 schrieb Dongsheng Yang: >> This commit make the ubifs_[get|set]xattr protected by ui_mutex. >> >> Originally, there is a possibility that ubifs_getxattr to get >> a wrong value. >> >> P1 P2 >> ---------- ---------- >> ubifs_getxattr ubifs_setxattr >> - kfree() >> - memcpy() >> - kmemdup() >> >> Then ubifs_getxattr() would get a non-sense data. To solve this >> problem, this commit make the xattr of ubifs_inode updated in >> atomic. > > so, ui->data needs protection? > The comment in fs/ubifs/ubifs.h does not mention ->data. > I'm asking because I want to make sure that ui_mutex is the correct lock to take. ui->data needs protection for sure, as I show above. Without protection, there is a problem to get wrong value. And yes, the comment does not mention the ->data. But I think ui_mutex is a good choice for it. And not all fields which is being protected by ui_mutex are listed in the comment, such as xattr_names and xattr_cnt. > >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >> --- >> -v2: >> Add more description in commit message >> fs/ubifs/xattr.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c >> index 96f3448..dec1afd 100644 >> --- a/fs/ubifs/xattr.c >> +++ b/fs/ubifs/xattr.c >> @@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, >> if (err) >> return err; >> >> + mutex_lock(&ui->ui_mutex); >> kfree(ui->data); >> ui->data = kmemdup(value, size, GFP_NOFS); >> if (!ui->data) { >> @@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, >> } >> inode->i_size = ui->ui_size = size; >> ui->data_len = size; >> + mutex_unlock(&ui->ui_mutex); > > You also need a mutex_unlock(&ui->ui_mutex) under out_free, otherwise > the if (!ui->data) { branch will trigger a deadlock. Wow, Great!!!! Sorry for my careless. Yang > > Thanks, > //richard > . >
Am 07.08.2015 um 07:40 schrieb Dongsheng Yang: > On 08/04/2015 04:27 AM, Richard Weinberger wrote: >> Am 31.07.2015 um 03:12 schrieb Dongsheng Yang: >>> This commit make the ubifs_[get|set]xattr protected by ui_mutex. >>> >>> Originally, there is a possibility that ubifs_getxattr to get >>> a wrong value. >>> >>> P1 P2 >>> ---------- ---------- >>> ubifs_getxattr ubifs_setxattr >>> - kfree() >>> - memcpy() >>> - kmemdup() >>> >>> Then ubifs_getxattr() would get a non-sense data. To solve this >>> problem, this commit make the xattr of ubifs_inode updated in >>> atomic. >> >> so, ui->data needs protection? >> The comment in fs/ubifs/ubifs.h does not mention ->data. >> I'm asking because I want to make sure that ui_mutex is the correct lock to take. > > ui->data needs protection for sure, as I show above. > Without protection, there is a problem to get wrong value. > > And yes, the comment does not mention the ->data. But > I think ui_mutex is a good choice for it. And not all fields > which is being protected by ui_mutex are listed in the comment, > such as xattr_names and xattr_cnt. Yeah. ;-\ The comment needs an update. In UBI and UBIFS we try hard to document our locking. Will you do a patch or shall I? Thanks, //richard
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index 96f3448..dec1afd 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, if (err) return err; + mutex_lock(&ui->ui_mutex); kfree(ui->data); ui->data = kmemdup(value, size, GFP_NOFS); if (!ui->data) { @@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, } inode->i_size = ui->ui_size = size; ui->data_len = size; + mutex_unlock(&ui->ui_mutex); mutex_lock(&host_ui->ui_mutex); host->i_ctime = ubifs_current_time(host); @@ -409,6 +411,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf, ubifs_assert(inode->i_size == ui->data_len); ubifs_assert(ubifs_inode(host)->xattr_size > ui->data_len); + mutex_lock(&ui->ui_mutex); if (buf) { /* If @buf is %NULL we are supposed to return the length */ if (ui->data_len > size) { @@ -423,6 +426,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf, err = ui->data_len; out_iput: + mutex_unlock(&ui->ui_mutex); iput(inode); out_unlock: kfree(xent);
This commit make the ubifs_[get|set]xattr protected by ui_mutex. Originally, there is a possibility that ubifs_getxattr to get a wrong value. P1 P2 ---------- ---------- ubifs_getxattr ubifs_setxattr - kfree() - memcpy() - kmemdup() Then ubifs_getxattr() would get a non-sense data. To solve this problem, this commit make the xattr of ubifs_inode updated in atomic. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- -v2: Add more description in commit message fs/ubifs/xattr.c | 4 ++++ 1 file changed, 4 insertions(+)