* [PATCH] staging: lustre: llite: replace variable length array
@ 2018-01-27 21:42 Sven Dziadek
2018-01-30 2:04 ` [lustre-devel] " Dilger, Andreas
0 siblings, 1 reply; 7+ messages in thread
From: Sven Dziadek @ 2018-01-27 21:42 UTC (permalink / raw)
To: gregkh, oleg.drokin, andreas.dilger, jsimmons
Cc: devel, dmitry.eremin, linux-kernel, lustre-devel
The functionality of the removed variable length array is already
implemented by the function xattr_full_name in fs/xattr.c
This fixes the sparse warning:
warning: Variable length array is used.
Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
---
drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
index 532384c91447..4fd28213c6a1 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
const char *name, const void *value, size_t size,
int flags)
{
- char fullname[strlen(handler->prefix) + strlen(name) + 1];
struct ll_sb_info *sbi = ll_i2sbi(inode);
struct ptlrpc_request *req = NULL;
const char *pv = value;
@@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
return -EPERM;
}
- sprintf(fullname, "%s%s\n", handler->prefix, name);
- rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
- valid, fullname, pv, size, 0, flags,
+ rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
+ xattr_full_name(handler, name), pv, size, 0, flags,
ll_i2suppgid(inode), &req);
if (rc) {
if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
@@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
struct dentry *dentry, struct inode *inode,
const char *name, void *buffer, size_t size)
{
- char fullname[strlen(handler->prefix) + strlen(name) + 1];
struct ll_sb_info *sbi = ll_i2sbi(inode);
#ifdef CONFIG_FS_POSIX_ACL
struct ll_inode_info *lli = ll_i2info(inode);
@@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
return -ENODATA;
#endif
- sprintf(fullname, "%s%s\n", handler->prefix, name);
- return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
- OBD_MD_FLXATTR);
+ return ll_xattr_list(inode, xattr_full_name(handler, name),
+ handler->flags, buffer, size, OBD_MD_FLXATTR);
}
static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
--
2.11.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: lustre: llite: replace variable length array
2018-01-27 21:42 [PATCH] staging: lustre: llite: replace variable length array Sven Dziadek
@ 2018-01-30 2:04 ` Dilger, Andreas
0 siblings, 0 replies; 7+ messages in thread
From: Dilger, Andreas @ 2018-01-30 2:04 UTC (permalink / raw)
To: Sven Dziadek, James Simmons
Cc: devel, Eremin, Dmitry, Greg Kroah-Hartman, lkml, Drokin, Oleg,
lustre-devel
On Jan 27, 2018, at 14:42, Sven Dziadek <sven.dziadek@gmx.de> wrote:
>
> The functionality of the removed variable length array is already
> implemented by the function xattr_full_name in fs/xattr.c
>
> This fixes the sparse warning:
> warning: Variable length array is used.
>
> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
> ---
> drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..4fd28213c6a1 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> const char *name, const void *value, size_t size,
> int flags)
> {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> struct ll_sb_info *sbi = ll_i2sbi(inode);
> struct ptlrpc_request *req = NULL;
> const char *pv = value;
> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> return -EPERM;
> }
>
> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> - rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> - valid, fullname, pv, size, 0, flags,
> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
> + xattr_full_name(handler, name), pv, size, 0, flags,
> ll_i2suppgid(inode), &req);
Hi Sven,
thanks for the patch.
Looking at the details of "xattr_full_name()", this seems quite risky. This
is essentially returning the pointer _before_ "name" on the assumption that
it contains the full "prefix.name" string. IMHO, that is not necessarily a
safe assumption to make several layers down in the code.
James, I thought you had a patch for this to use kasprintf() instead of the
on-stack "fullname" declaration?
Cheers, Andreas
> if (rc) {
> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> const char *name, void *buffer, size_t size)
> {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> struct ll_sb_info *sbi = ll_i2sbi(inode);
> #ifdef CONFIG_FS_POSIX_ACL
> struct ll_inode_info *lli = ll_i2info(inode);
> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
> return -ENODATA;
> #endif
> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> - return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
> - OBD_MD_FLXATTR);
> + return ll_xattr_list(inode, xattr_full_name(handler, name),
> + handler->flags, buffer, size, OBD_MD_FLXATTR);
> }
>
> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
> --
> 2.11.0
>
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: llite: replace variable length array
@ 2018-01-30 2:04 ` Dilger, Andreas
0 siblings, 0 replies; 7+ messages in thread
From: Dilger, Andreas @ 2018-01-30 2:04 UTC (permalink / raw)
To: Sven Dziadek, James Simmons
Cc: devel, Eremin, Dmitry, Greg Kroah-Hartman, lkml, Drokin, Oleg,
lustre-devel
On Jan 27, 2018, at 14:42, Sven Dziadek <sven.dziadek@gmx.de> wrote:
>
> The functionality of the removed variable length array is already
> implemented by the function xattr_full_name in fs/xattr.c
>
> This fixes the sparse warning:
> warning: Variable length array is used.
>
> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
> ---
> drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..4fd28213c6a1 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> const char *name, const void *value, size_t size,
> int flags)
> {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> struct ll_sb_info *sbi = ll_i2sbi(inode);
> struct ptlrpc_request *req = NULL;
> const char *pv = value;
> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> return -EPERM;
> }
>
> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> - rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> - valid, fullname, pv, size, 0, flags,
> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
> + xattr_full_name(handler, name), pv, size, 0, flags,
> ll_i2suppgid(inode), &req);
Hi Sven,
thanks for the patch.
Looking at the details of "xattr_full_name()", this seems quite risky. This
is essentially returning the pointer _before_ "name" on the assumption that
it contains the full "prefix.name" string. IMHO, that is not necessarily a
safe assumption to make several layers down in the code.
James, I thought you had a patch for this to use kasprintf() instead of the
on-stack "fullname" declaration?
Cheers, Andreas
> if (rc) {
> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> const char *name, void *buffer, size_t size)
> {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> struct ll_sb_info *sbi = ll_i2sbi(inode);
> #ifdef CONFIG_FS_POSIX_ACL
> struct ll_inode_info *lli = ll_i2info(inode);
> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
> return -ENODATA;
> #endif
> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> - return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
> - OBD_MD_FLXATTR);
> + return ll_xattr_list(inode, xattr_full_name(handler, name),
> + handler->flags, buffer, size, OBD_MD_FLXATTR);
> }
>
> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
> --
> 2.11.0
>
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: lustre: llite: replace variable length array
2018-01-30 2:04 ` [lustre-devel] " Dilger, Andreas
@ 2018-02-04 11:24 ` Sven Dziadek
-1 siblings, 0 replies; 7+ messages in thread
From: Sven Dziadek @ 2018-02-04 11:24 UTC (permalink / raw)
To: Dilger, Andreas, James Simmons
Cc: devel, Eremin, Dmitry, Greg Kroah-Hartman, lkml, Drokin, Oleg,
lustre-devel
On 01/30/2018 03:04 AM, Dilger, Andreas wrote:
> On Jan 27, 2018, at 14:42, Sven Dziadek <sven.dziadek@gmx.de> wrote:
>>
>> The functionality of the removed variable length array is already
>> implemented by the function xattr_full_name in fs/xattr.c
>>
>> This fixes the sparse warning:
>> warning: Variable length array is used.
>>
>> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
>> ---
>> drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
>> index 532384c91447..4fd28213c6a1 100644
>> --- a/drivers/staging/lustre/lustre/llite/xattr.c
>> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
>> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>> const char *name, const void *value, size_t size,
>> int flags)
>> {
>> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
>> struct ll_sb_info *sbi = ll_i2sbi(inode);
>> struct ptlrpc_request *req = NULL;
>> const char *pv = value;
>> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>> return -EPERM;
>> }
>>
>> - sprintf(fullname, "%s%s\n", handler->prefix, name);
>> - rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
>> - valid, fullname, pv, size, 0, flags,
>> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
>> + xattr_full_name(handler, name), pv, size, 0, flags,
>> ll_i2suppgid(inode), &req);
>
> Hi Sven,
> thanks for the patch.
>
> Looking at the details of "xattr_full_name()", this seems quite risky. This
> is essentially returning the pointer _before_ "name" on the assumption that
> it contains the full "prefix.name" string. IMHO, that is not necessarily a
> safe assumption to make several layers down in the code.
Thanks for your reply. And yeah, it feels strange, right.
But it really looks like this is how the name and the prefix is handled
in the xattr code.
It seems this helper function has been introduced with commit
e409de992e3ea (which also removes some fullname pointer) and indeed,
fs/xattr.c splits the prefix and the name in xattr_resolve_name.
So I still think the patch should be correct..?
> James, I thought you had a patch for this to use kasprintf() instead of the
> on-stack "fullname" declaration?
Sorry for replying that late, I though James would maybe know if the
above assumption is correct..
>> if (rc) {
>> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
>> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
>> struct dentry *dentry, struct inode *inode,
>> const char *name, void *buffer, size_t size)
>> {
>> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
>> struct ll_sb_info *sbi = ll_i2sbi(inode);
>> #ifdef CONFIG_FS_POSIX_ACL
>> struct ll_inode_info *lli = ll_i2info(inode);
>> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
>> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
>> return -ENODATA;
>> #endif
>> - sprintf(fullname, "%s%s\n", handler->prefix, name);
>> - return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
>> - OBD_MD_FLXATTR);
>> + return ll_xattr_list(inode, xattr_full_name(handler, name),
>> + handler->flags, buffer, size, OBD_MD_FLXATTR);
>> }
>>
>> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
>> --
>> 2.11.0
>>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: llite: replace variable length array
@ 2018-02-04 11:24 ` Sven Dziadek
0 siblings, 0 replies; 7+ messages in thread
From: Sven Dziadek @ 2018-02-04 11:24 UTC (permalink / raw)
To: Dilger, Andreas, James Simmons
Cc: devel, Eremin, Dmitry, Greg Kroah-Hartman, lkml, Drokin, Oleg,
lustre-devel
On 01/30/2018 03:04 AM, Dilger, Andreas wrote:
> On Jan 27, 2018, at 14:42, Sven Dziadek <sven.dziadek@gmx.de> wrote:
>>
>> The functionality of the removed variable length array is already
>> implemented by the function xattr_full_name in fs/xattr.c
>>
>> This fixes the sparse warning:
>> warning: Variable length array is used.
>>
>> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
>> ---
>> drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
>> index 532384c91447..4fd28213c6a1 100644
>> --- a/drivers/staging/lustre/lustre/llite/xattr.c
>> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
>> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>> const char *name, const void *value, size_t size,
>> int flags)
>> {
>> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
>> struct ll_sb_info *sbi = ll_i2sbi(inode);
>> struct ptlrpc_request *req = NULL;
>> const char *pv = value;
>> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>> return -EPERM;
>> }
>>
>> - sprintf(fullname, "%s%s\n", handler->prefix, name);
>> - rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
>> - valid, fullname, pv, size, 0, flags,
>> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
>> + xattr_full_name(handler, name), pv, size, 0, flags,
>> ll_i2suppgid(inode), &req);
>
> Hi Sven,
> thanks for the patch.
>
> Looking at the details of "xattr_full_name()", this seems quite risky. This
> is essentially returning the pointer _before_ "name" on the assumption that
> it contains the full "prefix.name" string. IMHO, that is not necessarily a
> safe assumption to make several layers down in the code.
Thanks for your reply. And yeah, it feels strange, right.
But it really looks like this is how the name and the prefix is handled
in the xattr code.
It seems this helper function has been introduced with commit
e409de992e3ea (which also removes some fullname pointer) and indeed,
fs/xattr.c splits the prefix and the name in xattr_resolve_name.
So I still think the patch should be correct..?
> James, I thought you had a patch for this to use kasprintf() instead of the
> on-stack "fullname" declaration?
Sorry for replying that late, I though James would maybe know if the
above assumption is correct..
>> if (rc) {
>> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
>> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
>> struct dentry *dentry, struct inode *inode,
>> const char *name, void *buffer, size_t size)
>> {
>> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
>> struct ll_sb_info *sbi = ll_i2sbi(inode);
>> #ifdef CONFIG_FS_POSIX_ACL
>> struct ll_inode_info *lli = ll_i2info(inode);
>> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
>> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
>> return -ENODATA;
>> #endif
>> - sprintf(fullname, "%s%s\n", handler->prefix, name);
>> - return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
>> - OBD_MD_FLXATTR);
>> + return ll_xattr_list(inode, xattr_full_name(handler, name),
>> + handler->flags, buffer, size, OBD_MD_FLXATTR);
>> }
>>
>> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
>> --
>> 2.11.0
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: lustre: llite: replace variable length array
2018-02-04 11:24 ` [lustre-devel] " Sven Dziadek
@ 2018-02-11 21:38 ` James Simmons
-1 siblings, 0 replies; 7+ messages in thread
From: James Simmons @ 2018-02-11 21:38 UTC (permalink / raw)
To: Sven Dziadek
Cc: devel, Eremin, Dmitry, Dilger, Andreas, Greg Kroah-Hartman, lkml,
Drokin, Oleg, lustre-devel
> On 01/30/2018 03:04 AM, Dilger, Andreas wrote:
> > On Jan 27, 2018, at 14:42, Sven Dziadek <sven.dziadek@gmx.de> wrote:
> >>
> >> The functionality of the removed variable length array is already
> >> implemented by the function xattr_full_name in fs/xattr.c
> >>
> >> This fixes the sparse warning:
> >> warning: Variable length array is used.
> >>
> >> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
> >> ---
> >> drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
> >> 1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> >> index 532384c91447..4fd28213c6a1 100644
> >> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> >> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> >> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> >> const char *name, const void *value, size_t size,
> >> int flags)
> >> {
> >> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> >> struct ll_sb_info *sbi = ll_i2sbi(inode);
> >> struct ptlrpc_request *req = NULL;
> >> const char *pv = value;
> >> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> >> return -EPERM;
> >> }
> >>
> >> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> >> - rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> >> - valid, fullname, pv, size, 0, flags,
> >> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
> >> + xattr_full_name(handler, name), pv, size, 0, flags,
> >> ll_i2suppgid(inode), &req);
> >
> > Hi Sven,
> > thanks for the patch.
> >
> > Looking at the details of "xattr_full_name()", this seems quite risky. This
> > is essentially returning the pointer _before_ "name" on the assumption that
> > it contains the full "prefix.name" string. IMHO, that is not necessarily a
> > safe assumption to make several layers down in the code.
> Thanks for your reply. And yeah, it feels strange, right.
>
> But it really looks like this is how the name and the prefix is handled
> in the xattr code.
> It seems this helper function has been introduced with commit
> e409de992e3ea (which also removes some fullname pointer) and indeed,
> fs/xattr.c splits the prefix and the name in xattr_resolve_name.
>
> So I still think the patch should be correct..?
>
> > James, I thought you had a patch for this to use kasprintf() instead of the
> > on-stack "fullname" declaration?
> Sorry for replying that late, I though James would maybe know if the
> above assumption is correct..
Yes I do have a patch for this. I pushed them several months ago but they
got missed. It happens. I will the patches again very soon.
> >> if (rc) {
> >> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
> >> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> >> struct dentry *dentry, struct inode *inode,
> >> const char *name, void *buffer, size_t size)
> >> {
> >> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> >> struct ll_sb_info *sbi = ll_i2sbi(inode);
> >> #ifdef CONFIG_FS_POSIX_ACL
> >> struct ll_inode_info *lli = ll_i2info(inode);
> >> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> >> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
> >> return -ENODATA;
> >> #endif
> >> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> >> - return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
> >> - OBD_MD_FLXATTR);
> >> + return ll_xattr_list(inode, xattr_full_name(handler, name),
> >> + handler->flags, buffer, size, OBD_MD_FLXATTR);
> >> }
> >>
> >> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
> >> --
> >> 2.11.0
> >>
>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: llite: replace variable length array
@ 2018-02-11 21:38 ` James Simmons
0 siblings, 0 replies; 7+ messages in thread
From: James Simmons @ 2018-02-11 21:38 UTC (permalink / raw)
To: Sven Dziadek
Cc: devel, Eremin, Dmitry, Dilger, Andreas, Greg Kroah-Hartman, lkml,
Drokin, Oleg, lustre-devel
> On 01/30/2018 03:04 AM, Dilger, Andreas wrote:
> > On Jan 27, 2018, at 14:42, Sven Dziadek <sven.dziadek@gmx.de> wrote:
> >>
> >> The functionality of the removed variable length array is already
> >> implemented by the function xattr_full_name in fs/xattr.c
> >>
> >> This fixes the sparse warning:
> >> warning: Variable length array is used.
> >>
> >> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
> >> ---
> >> drivers/staging/lustre/lustre/llite/xattr.c | 12 ++++--------
> >> 1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> >> index 532384c91447..4fd28213c6a1 100644
> >> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> >> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> >> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> >> const char *name, const void *value, size_t size,
> >> int flags)
> >> {
> >> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> >> struct ll_sb_info *sbi = ll_i2sbi(inode);
> >> struct ptlrpc_request *req = NULL;
> >> const char *pv = value;
> >> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
> >> return -EPERM;
> >> }
> >>
> >> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> >> - rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> >> - valid, fullname, pv, size, 0, flags,
> >> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
> >> + xattr_full_name(handler, name), pv, size, 0, flags,
> >> ll_i2suppgid(inode), &req);
> >
> > Hi Sven,
> > thanks for the patch.
> >
> > Looking at the details of "xattr_full_name()", this seems quite risky. This
> > is essentially returning the pointer _before_ "name" on the assumption that
> > it contains the full "prefix.name" string. IMHO, that is not necessarily a
> > safe assumption to make several layers down in the code.
> Thanks for your reply. And yeah, it feels strange, right.
>
> But it really looks like this is how the name and the prefix is handled
> in the xattr code.
> It seems this helper function has been introduced with commit
> e409de992e3ea (which also removes some fullname pointer) and indeed,
> fs/xattr.c splits the prefix and the name in xattr_resolve_name.
>
> So I still think the patch should be correct..?
>
> > James, I thought you had a patch for this to use kasprintf() instead of the
> > on-stack "fullname" declaration?
> Sorry for replying that late, I though James would maybe know if the
> above assumption is correct..
Yes I do have a patch for this. I pushed them several months ago but they
got missed. It happens. I will the patches again very soon.
> >> if (rc) {
> >> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
> >> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> >> struct dentry *dentry, struct inode *inode,
> >> const char *name, void *buffer, size_t size)
> >> {
> >> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> >> struct ll_sb_info *sbi = ll_i2sbi(inode);
> >> #ifdef CONFIG_FS_POSIX_ACL
> >> struct ll_inode_info *lli = ll_i2info(inode);
> >> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
> >> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
> >> return -ENODATA;
> >> #endif
> >> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> >> - return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
> >> - OBD_MD_FLXATTR);
> >> + return ll_xattr_list(inode, xattr_full_name(handler, name),
> >> + handler->flags, buffer, size, OBD_MD_FLXATTR);
> >> }
> >>
> >> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
> >> --
> >> 2.11.0
> >>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-02-11 21:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-27 21:42 [PATCH] staging: lustre: llite: replace variable length array Sven Dziadek
2018-01-30 2:04 ` Dilger, Andreas
2018-01-30 2:04 ` [lustre-devel] " Dilger, Andreas
2018-02-04 11:24 ` Sven Dziadek
2018-02-04 11:24 ` [lustre-devel] " Sven Dziadek
2018-02-11 21:38 ` James Simmons
2018-02-11 21:38 ` [lustre-devel] " James Simmons
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.