fs/xattr.c:882 __simple_xattr_set() error: potential NULL dereference 'new_xattr'.

Hi Aristeu,

FYI, there are new smatch warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-next
head:   9814e970d7947dcc5ab7b37a53514c0098bfacc9
commit: 38f38657444d15e1a8574eae80ed3de9f501737a xattr: extract simple_xattr code from tmpfs

fs/xattr.c:882 __simple_xattr_set() error: potential NULL dereference 'new_xattr'.

vim +882 fs/xattr.c
872 } else {
873 list_del(&xattr->list);
874 }
875 goto out;
876 }
877 }
878 if (flags & XATTR_REPLACE) {
879 xattr = new_xattr;
880 err = -ENODATA;
881 } else {

883 xattr = NULL;
884 }
885 out:
886 spin_unlock(&xattrs->lock);
887 if (xattr) {
888 kfree(xattr->name);
889 kfree(xattr);
890 }
891 return err;
892

---
0-DAY kernel build testing backend Open Source Technology Centre
Fengguang Wu <wfg-VuQAYsv1563Yd54FQh9/[email protected]> Intel Corporation

I don't know if this specific code is buggy or not. It would depend on how the function is called.

But potentially I should disable this Smatch rule. It tends to have a lot of false positives. The thing is that GCC complains if you don't initialize "new_xattr", but if you initialize it to NULL then Smatch complains.

One solution might be to use the unitialized_var() macro.

- struct simple_xattr *new_xattr = NULL;
+ struct simple_xattr *uninitialized_var(new_xattr);

That would make both GCC and Smatch happy.

regards,
dan carpenter

this should be safe. the only way to have value == NULL (thus keeping new_xattr from being initialized) is if you call __simple_xattr_set() directly with the intention of removing an existing entry.

Sounds good to me. Will get a patch ready. Thanks Dan.

Aristeu

new_xattr in __simple_xattr_set() is only initialized with a valid pointer if value is not NULL, which only happens if this function is called directly with the intention to remove an existing extended attribute. Even being safe to be this way, smatch warns about possible NULL dereference. Dan Carpenter suggested using uninitialized_var() which will make both gcc and smatch happy.

---
 fs/xattr.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -845,7 +845,7 @@ static int __simple_xattr_set(struct sim
 			      const void *value, size_t size, int flags)
 {
 	struct simple_xattr *xattr;
-	struct simple_xattr *new_xattr = NULL;
+	struct simple_xattr *uninitialized_var(new_xattr);
 	int err = 0;
 
 	/* value == NULL means remove */

applied to cgroup/for-3.7. Thanks.

tejun