Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection
On Tue, Nov 24, 2015 at 09:21:06PM +0100, Ilya Dryomov wrote:
> >> Cleanup here is (and should be) done in reverse order.
> >
Yes. This is true.
> > I have got an other impression about the appropriate order for the
> > corresponding
> > clean-up function calls.
> >
> >
> >> We allocate parent rbd_device and then link it with what we already have,
> >
> > I guess that we have got a different understanding about the relevant
> > "linking".
>
> Well, there isn't any _literal_ linking (e.g. adding to a link list,
> etc) in this case. We just bump some refs and do probe to fill in the
> newly allocated parent. If probe fails, we put refs and free parent,
> reversing the "alloc parent, bump refs" order.
>
> The actual linking (rbd_dev->parent = parent) is done right before
> returning so we never have to undo it in rbd_dev_probe_parent() and
> that's the only reason your patch probably doesn't break anything.
> Think about what happens if, after your patch is applied, someone moves
> that assignment up or adds an extra step that can fail after it...
>
The problem is that the unwind code should be a mirror of the allocate
code but rbd_dev_unparent() doesn't mirror anything. Generally, writing
future proof stubs like this is a wrong thing because predicting the
future is hard and in the mean time we are left stubs which confuse
everyone.
> If all error paths could be adjusted so that NULL pointers are never
> passed in, destroy functions wouldn't need to have a NULL check, would
> they?
Yep. We agree on the right way to do it. I am probably the number one
kernel developer for removing the most sanity checks. :P (As opposed
to patch 1/1 where we now rely on the sanity check inside
rbd_dev_destroy().)
drivers/block/rbd.c
5149 static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
5150 {
5151 struct rbd_device *parent = NULL;
5152 int ret;
5153
5154 if (!rbd_dev->parent_spec)
5155 return 0;
5156
5157 if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
5158 pr_info("parent chain is too long (%d)\n", depth);
5159 ret = -EINVAL;
5160 goto out_err;
We haven't allocated anything so this should just be return -EINVAL;
In the original code, we decrement the kref count on ->parent_spec on
this error path so that is a classic One Err Bug.
5161 }
5162
5163 parent = rbd_dev_create(rbd_dev->rbd_client,
rbd_dev->parent_spec,
5164 NULL);
5165 if (!parent) {
5166 ret = -ENOMEM;
5167 goto out_err;
Still haven't allocated anything so return -ENOMEM, but if we fail after
this point we will need to call rbd_dev_destroy().
5168 }
5169
5170 /*
5171 * Images related by parent/child relationships always share
5172 * rbd_client and spec/parent_spec, so bump their refcounts.
5173 */
5174 __rbd_get_client(rbd_dev->rbd_client);
5175 rbd_spec_get(rbd_dev->parent_spec);
We will need to put these on any later error paths.
5176
5177 ret = rbd_dev_image_probe(parent, depth);
5178 if (ret < 0)
5179 goto out_err;
Ok. We need to put the ->parent_spec, ->rbd_client and free the parent.
5180
5181 rbd_dev->parent = parent;
5182 atomic_set(_dev->parent_ref, 1);
5183 return 0;
5184
5185 out_err:
5186 rbd_dev_unparent(rbd_dev);
This is a complicated way to say rbd_spec_put(rbd_dev->parent_spec);
Also, is it really necessary to set ->parent_spec to NULL? If we didn't
put the last reference then doesn't setting it to NULL mean we are
leaking? Setting it to NULL is confusing and feels like a layering
violation.
5187 if (parent)
5188 rbd_dev_destroy(parent);
5189 return ret;
5190 }
I feel like we should be calling rbd_put_client() on this error path or
else the code is buggy or has layer violations. So I *think* it should
look like this:
dec_ref_counts:
rbd_spec_put(rbd_dev->parent_spec);
rbd_put_client(rbd_dev->rbd_client);
rbd_dev_destroy(parent);
return ret;
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: cope with out of order (unsafe after safe) mds reply
Hello Sage Weil,
The patch 85792d0dd6e7: ceph: cope with out of order (unsafe after
safe) mds reply from May 13, 2010, leads to the following static
checker warning:
fs/ceph/mds_client.c:2414 handle_reply()
warn: we tested 'head-safe' before and it was 'false'
fs/ceph/mds_client.c
2406 /* dup? */
2407 if ((req-r_got_unsafe !head-safe) ||
2408 (req-r_got_safe head-safe)) {
^^^^^
If -r_got_safe is set we always fail. If head-safe is set then we
fail here.
2409 pr_warn(got a dup %s reply on %llu from mds%d\n,
2410 head-safe ? safe : unsafe, tid, mds);
2411 mutex_unlock(mdsc-mutex);
2412 goto out;
2413 }
2414 if (req-r_got_safe !head-safe) {
Otherwise we fail here. The only thing different is the error message.
2415 pr_warn(got unsafe after safe on %llu from mds%d\n,
2416 tid, mds);
2417 mutex_unlock(mdsc-mutex);
2418 goto out;
2419 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] ceph: do_sync is never initialized
Probably this code was syncing a lot more often then intended because
the do_sync variable wasn't set to zero.
Fixes: c62988ec0910 ('ceph: avoid meaningless calling ceph_caps_revoking if
sync_mode == WB_SYNC_ALL.')
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
---
Static checker stuff. Not tested. It's surprising that GCC doesn't
catch this.
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 18c06bb..481529b 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -673,7 +673,7 @@ static int ceph_writepages_start(struct address_space
*mapping,
int rc = 0;
unsigned wsize = 1 inode-i_blkbits;
struct ceph_osd_request *req = NULL;
- int do_sync;
+ int do_sync = 0;
u64 truncate_size, snap_size;
u32 truncate_seq;
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: fix fallocate division
Hello Sage Weil,
The patch b314a90d8f3f: ceph: fix fallocate division from Aug 27,
2013, leads to the following static checker warning:
fs/ceph/file.c:1145 ceph_zero_objects()
warn: [initializer] should 'object_size * stripe_count' be a 64 bit
type?
fs/ceph/file.c
1138 static int ceph_zero_objects(struct inode *inode, loff_t offset, loff_t
length)
1139 {
1140 int ret = 0;
1141 struct ceph_inode_info *ci = ceph_inode(inode);
1142 s32 stripe_unit = ceph_file_layout_su(ci-i_layout);
1143 s32 stripe_count = ceph_file_layout_stripe_count(ci-i_layout);
1144 s32 object_size = ceph_file_layout_object_size(ci-i_layout);
1145 u64 object_set_size = object_size * stripe_count;
^^
This is a new Smatch check I'm working on. Obviously integer overflows
are a fairly common bug. We often see code like this:
a = b * c;
It's too much to warn every time when the types don't make sense because
a lot of times people use u64 by reflexive and don't actually care about
the upper bits. My new check only warns if the overflow happens inside
an initializer.
This one is puzzling to me because prior to b314a90d8f3f then there was
a cast that fixed the integer overflow problem. It's not clear if
removing it was intentional or accidental.
1146 u64 nearly, t;
1147
1148 /* round offset up to next period boundary */
1149 nearly = offset + object_set_size - 1;
1150 t = nearly;
1151 nearly -= do_div(t, object_set_size);
1152
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: rbd: use watch/notify for changes in rbd header
On Thu, Feb 13, 2014 at 06:27:41AM -0600, Alex Elder wrote:
On 02/13/2014 06:04 AM, Ilya Dryomov wrote:
On Thu, Feb 13, 2014 at 11:50 AM, Dan Carpenter
dan.carpen...@oracle.com wrote:
Hello Yehuda Sadeh,
The patch 59c2be1e4d42: rbd: use watch/notify for changes in rbd
header from Mar 21, 2011, leads to the following static checker
warning:
drivers/block/rbd.c:687 parse_rbd_opts_token()
warn: we tested 'token 0' before and it was 'false'
drivers/block/rbd.c
677 static int parse_rbd_opts_token(char *c, void *private)
678 {
679 struct rbd_options *rbd_opts = private;
680 substring_t argstr[MAX_OPT_ARGS];
681 int token, intval, ret;
682
683 token = match_token(c, rbd_opts_tokens, argstr);
684 if (token 0)
^
685 return -EINVAL;
686
687 if (token Opt_last_int) {
Opt_last_int is zero so this is never true. Should the be == or
something?
Probably not. It's there in case an integer option gets added to rbd
in the future. See parse_fsopt_token() and the enums above it in
fs/ceph/super.c.
Yes, that's correct. There were once integer options and this
code is a holdover from that time.
I think it should remain in case it's needed, but it could be
#ifdef'd or something. (Or we could do some magic to get the
warning to go away.)
-Alex
These warnings are a one time thing. No need for ifdefs.
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: handle frag mismatch between readdir request and reply
Hello Yan, Zheng,
This is a semi-automatic email about new static checker warnings.
The patch d1813cc074de: ceph: handle frag mismatch between readdir
request and reply from Sep 18, 2013, leads to the following Smatch
complaint:
fs/ceph/inode.c:1312 ceph_readdir_prepopulate()
warn: variable dereferenced before check 'rinfo-dir_dir' (see line
1298)
fs/ceph/inode.c
1291 if (le32_to_cpu(rinfo-dir_dir-frag) != frag) {
Patch introduces a dereference.
1292 dout(readdir_prepopulate got new frag %x - %x\n,
1293 frag, le32_to_cpu(rinfo-dir_dir-frag));
1294 frag = le32_to_cpu(rinfo-dir_dir-frag);
1295 if (ceph_frag_is_leftmost(frag))
1296 r_readdir_offset = 2;
1297 else
1298 r_readdir_offset = 0;
1299 }
1300
1301 if (req-r_aborted)
1302 return readdir_prepopulate_inodes_only(req, session);
1303
1304 if (le32_to_cpu(rinfo-head-op) == CEPH_MDS_OP_LSSNAP) {
1305 snapdir = ceph_get_snapdir(parent-d_inode);
1306 parent = d_find_alias(snapdir);
1307 dout(readdir_prepopulate %d items under SNAPDIR dn
%p\n,
1308 rinfo-dir_nr, parent);
1309 } else {
1310 dout(readdir_prepopulate %d items under dn %p\n,
1311 rinfo-dir_nr, parent);
1312 if (rinfo-dir_dir)
^^
Old check.
1313 ceph_fill_dirfrag(parent-d_inode,
rinfo-dir_dir);
1314 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: remove outdated frag information
Hello Yan, Zheng,
The patch 947c4a83bd2f: ceph: remove outdated frag information from
Sep 18, 2013, leads to the following
static checker warning: fs/ceph/inode.c:788 fill_inode()
warn: 'frag' was already freed.
fs/ceph/inode.c
769 frag = NULL;
770 while (rb_node) {
771 frag = rb_entry(rb_node, struct
ceph_inode_frag, node);
772 if (ceph_frag_compare(frag-frag, id) = 0) {
773 if (frag-frag != id)
774 frag = NULL;
775 else
776 rb_node = rb_next(rb_node);
777 break;
778 }
779 rb_node = rb_next(rb_node);
780 rb_erase(frag-node, ci-i_fragtree);
781 kfree(frag);
^^^
kfree here.
782 }
783 if (!frag) {
784 frag = __get_or_create_frag(ci, id);
785 if (IS_ERR(frag))
786 continue;
787 }
788 frag-split_by =
le32_to_cpu(info-fragtree.splits[i].by);
^^
Potential use after free.
789 dout( frag %x split by %d\n, frag-frag,
frag-split_by);
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 3/2] libceph: create_singlethread_workqueue() doesn't return ERR_PTRs
create_singlethread_workqueue() returns NULL on error, and it doesn't
return ERR_PTRs.
I tweaked the error handling a little to be consistent with earlier in
the function.
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index dbc0a73..02c5246 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -2257,12 +2257,10 @@ int ceph_osdc_init(struct ceph_osd_client *osdc, struct
ceph_client *client)
if (err 0)
goto out_msgpool;
+ err = -ENOMEM;
osdc-notify_wq = create_singlethread_workqueue(ceph-watch-notify);
- if (IS_ERR(osdc-notify_wq)) {
- err = PTR_ERR(osdc-notify_wq);
- osdc-notify_wq = NULL;
+ if (!osdc-notify_wq)
goto out_msgpool;
- }
return 0;
out_msgpool:
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 1/2] libceph: fix error handling in handle_reply()
We've tried to fix the error paths in this function before, but there
is still a hidden goto in the ceph_decode_need() macro which goes to the
wrong place. We need to release the req and unlock a mutex before
returning.
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index dbc0a73..559a832 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1488,14 +1488,14 @@ static void handle_reply(struct ceph_osd_client *osdc,
struct ceph_msg *msg,
dout(handle_reply %p tid %llu req %p result %d\n, msg, tid,
req, result);
- ceph_decode_need(p, end, 4, bad);
+ ceph_decode_need(p, end, 4, bad_put);
numops = ceph_decode_32(p);
if (numops CEPH_OSD_MAX_OP)
goto bad_put;
if (numops != req-r_num_ops)
goto bad_put;
payload_len = 0;
- ceph_decode_need(p, end, numops * sizeof(struct ceph_osd_op), bad);
+ ceph_decode_need(p, end, numops * sizeof(struct ceph_osd_op), bad_put);
for (i = 0; i numops; i++) {
struct ceph_osd_op *op = p;
int len;
@@ -1513,7 +1513,7 @@ static void handle_reply(struct ceph_osd_client *osdc,
struct ceph_msg *msg,
goto bad_put;
}
- ceph_decode_need(p, end, 4 + numops * 4, bad);
+ ceph_decode_need(p, end, 4 + numops * 4, bad_put);
retry_attempt = ceph_decode_32(p);
for (i = 0; i numops; i++)
req-r_reply_op_result[i] = ceph_decode_32(p);
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 2/2] libceph: potential NULL dereference in ceph_osdc_handle_map()
There are two places where we read nr_maps if both of them are set to zero then we would hit a NULL dereference here. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com --- I think this is a real bug, but please review my fix for it because I'm not very familiar with this code. diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 559a832..56f1fe5 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -1786,6 +1786,8 @@ void ceph_osdc_handle_map(struct ceph_osd_client *osdc, struct ceph_msg *msg) nr_maps--; } + if (!osdc-osdmap) + goto bad; done: downgrade_write(osdc-map_sem); ceph_monc_got_osdmap(osdc-client-monc, osdc-osdmap-epoch); -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[parch] ceph: cleanup types in striped_read()
We pass in a u64 value for len and then immediately truncate away the
upper 32 bits.
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 2ddf061..93e53c8 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -313,9 +313,9 @@ static int striped_read(struct inode *inode,
{
struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
struct ceph_inode_info *ci = ceph_inode(inode);
- u64 pos, this_len;
+ u64 pos, this_len, left;
int io_align, page_align;
- int left, pages_left;
+ int pages_left;
int read;
struct page **page_pos;
int ret;
@@ -346,7 +346,7 @@ more:
ret = 0;
hit_stripe = this_len left;
was_short = ret = 0 ret this_len;
- dout(striped_read %llu~%u (read %u) got %d%s%s\n, pos, left, read,
+ dout(striped_read %llu~%llu (read %u) got %d%s%s\n, pos, left, read,
ret, hit_stripe ? HITSTRIPE : , was_short ? SHORT : );
if (ret 0) {
@@ -378,7 +378,7 @@ more:
if (pos + left inode-i_size)
left = inode-i_size - pos;
- dout(zero tail %d\n, left);
+ dout(zero tail %llu\n, left);
ceph_zero_page_vector_range(page_align + read, left,
pages);
read += left;
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: rbd: refactor rbd_header_from_disk()
Hello Alex Elder,
The patch bb23e37acb2a: rbd: refactor rbd_header_from_disk() from
May 6, 2013, has some integer overflow bugs:
drivers/block/rbd.c
793 snap_count = le32_to_cpu(ondisk-snap_count);
794 snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
snap_count comes from the disk. On 32 bit systems there is an integer
overflow problem inside ceph_create_snap_context() so snapc could be
smaller than intended.
795 if (!snapc)
796 goto out_err;
797 snapc-seq = le64_to_cpu(ondisk-snap_seq);
798 if (snap_count) {
799 struct rbd_image_snap_ondisk *snaps;
800 u64 snap_names_len =
le64_to_cpu(ondisk-snap_names_len);
801
802 /* We'll keep a copy of the snapshot names... */
803
804 if (snap_names_len (u64)SIZE_MAX)
805 goto out_2big;
806 snap_names = kmalloc(snap_names_len, GFP_KERNEL);
807 if (!snap_names)
808 goto out_err;
809
810 /* ...as well as the array of their sizes. */
811
812 size = snap_count * sizeof (*header-snap_sizes);
^
There is a second integer overflow bug here.
813 snap_sizes = kmalloc(size, GFP_KERNEL);
814 if (!snap_sizes)
815 goto out_err;
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] ceph: tidy ceph_mdsmap_decode() a little
I introduced a new temporary variable info instead of
m-m_info[mds]. Also I reversed the if condition and pulled
everything in one indent level.
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
---
This goes on top of Emil Goode's patch.
diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
index d4d3897..132b64e 100644
--- a/fs/ceph/mdsmap.c
+++ b/fs/ceph/mdsmap.c
@@ -92,6 +92,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
u32 num_export_targets;
void *pexport_targets = NULL;
struct ceph_timespec laggy_since;
+ struct ceph_mds_info *info;
ceph_decode_need(p, end, sizeof(u64)*2 + 1 + sizeof(u32), bad);
global_id = ceph_decode_64(p);
@@ -126,26 +127,27 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void
*end)
i+1, n, global_id, mds, inc,
ceph_pr_addr(addr.in_addr),
ceph_mds_state_name(state));
- if (mds = 0 mds m-m_max_mds state 0) {
- m-m_info[mds].global_id = global_id;
- m-m_info[mds].state = state;
- m-m_info[mds].addr = addr;
- m-m_info[mds].laggy =
- (laggy_since.tv_sec != 0 ||
-laggy_since.tv_nsec != 0);
- m-m_info[mds].num_export_targets = num_export_targets;
- if (num_export_targets) {
- m-m_info[mds].export_targets =
- kcalloc(num_export_targets, sizeof(u32),
- GFP_NOFS);
- if (m-m_info[mds].export_targets == NULL)
- goto badmem;
- for (j = 0; j num_export_targets; j++)
- m-m_info[mds].export_targets[j] =
- ceph_decode_32(pexport_targets);
- } else {
- m-m_info[mds].export_targets = NULL;
- }
+
+ if (mds 0 || mds = m-m_max_mds || state = 0)
+ continue;
+
+ info = m-m_info[mds];
+ info-global_id = global_id;
+ info-state = state;
+ info-addr = addr;
+ info-laggy = (laggy_since.tv_sec != 0 ||
+ laggy_since.tv_nsec != 0);
+ info-num_export_targets = num_export_targets;
+ if (num_export_targets) {
+ info-export_targets = kcalloc(num_export_targets,
+ sizeof(u32), GFP_NOFS);
+ if (info-export_targets == NULL)
+ goto badmem;
+ for (j = 0; j num_export_targets; j++)
+ info-export_targets[j] =
+ ceph_decode_32(pexport_targets);
+ } else {
+ info-export_targets = NULL;
}
}
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] ceph: tidy ceph_mdsmap_decode() a little
On Wed, May 29, 2013 at 09:17:21AM +0200, walter harms wrote: personally i would go for: info-export_targets = NULL; and remove the else below. I don't have strong feelings one way or the other, but honestly, I think the way I sent it is more clear. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] ceph: remove unneeded truncation
My static checker complains l-stripe_unit could still be zero after we truncate it to 32 bits. I don't see a reason to do the truncation so I have removed it. Both sides are u64 type. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c index e0b4ef3..98fe5e7 100644 --- a/fs/ceph/ioctl.c +++ b/fs/ceph/ioctl.c @@ -43,7 +43,7 @@ static long __validate_layout(struct ceph_mds_client *mdsc, if ((l-object_size ~PAGE_MASK) || (l-stripe_unit ~PAGE_MASK) || (l-stripe_unit != 0 -((unsigned)l-object_size % (unsigned)l-stripe_unit))) +(l-object_size % l-stripe_unit))) return -EINVAL; /* make sure it's a valid data pool */ -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: rbd: enforce parent overlap
On Tue, Apr 30, 2013 at 06:57:14AM -0500, Alex Elder wrote: On 04/30/2013 02:24 AM, Dan Carpenter wrote: Hello Alex Elder, This is a semi-automatic email about new static checker warnings. Cool, I've never used smatch before. Great to get these automated warnings. I looked at this, and this is not a problem. An earlier commit, about 10 before that one: rbd: always check IMG_DATA flag implements a comparable check. Any rbd object request with the IMG_DATA flag set (obj_request_img_data_test() returns true) will have a non-null image_request pointer. I suppose I should have asserted it was non-null, I do that all over the place... If Smatch knows a variable is never NULL then it ignores inconsistent NULL checking but it would be better to just remove the unneeded check. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: crush: warn on do_rule failure
Hello Sage Weil,
The patch 8b3932690084: crush: warn on do_rule failure from May 7,
2012, leads to the following warning:
net/ceph/osdmap.c:1117:8: warning: comparison of unsigned expression 0 is
always false [-Wtautological-compare]
net/ceph/osdmap.c
1114 r = crush_do_rule(osdmap-crush, ruleno, pps, osds,
1115min_t(int, pool-v.size, *num),
1116osdmap-osd_weight);
1117 if (r 0) {
^
r is unsigned so it's never less than zero. Also crush_do_rule() never
returns negative numbers.
1118 pr_err(error %d from crush rule: pool %d ruleset %d
type %d
1119 size %d\n, r, poolid, pool-v.crush_ruleset,
1120 pool-v.type, pool-v.size);
1121 return NULL;
1122 }
1123 *num = r;
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: fix encoding of ino only (not relative) paths
Hello Sage Weil,
This is a semi-automatic email about new static checker warnings.
The patch 795858dbd253: ceph: fix encoding of ino only (not
relative) paths from Aug 15, 2011, leads to the following Smatch
complaint:
fs/ceph/mds_client.c:1593 set_request_path_attr()
error: we previously assumed 'rpath' could be null (see line 1590)
fs/ceph/mds_client.c
1589 *ppath);
1590 } else if (rpath || rino) {
^
Check.
1591 *ino = rino;
1592 *ppath = rpath;
1593 *pathlen = strlen(rpath);
^
Dereference.
1594 dout( path %.*s\n, *pathlen, rpath);
1595 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: factor out libceph from Ceph file system
Hello Yehuda Sadeh, This is a semi-automatic email about new static checker warnings. The patch 3d14c5d2b6e1: ceph: factor out libceph from Ceph file system from Apr 6, 2010, leads to the following Smatch complaint: fs/ceph/super.c:279 strcmp_null() error: we previously assumed 's1' could be null (see line 277) fs/ceph/super.c 276 return -1; 277 if (!s1 s2) ^ Check. 278 return 1; 279 return strcmp(s1, s2); ^^ Dereference inside function call. 280 } 281 regards, dan carpenter See also: -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ceph: factor out libceph from Ceph file system
Oh crap. I didn't read this at all. Obviously a false positive. Sorry about that. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] ceph: divide by zero bug in __validate_layout()
If l-stripe_unit is zero the the mod on the next line will cause a divide by zero bug. This comes from the copy_from_user() in ceph_ioctl_set_layout_policy() Signed-off-by: Dan Carpenter dan.carpen...@oracle.com diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c index 8e3fb69..81ec22b 100644 --- a/fs/ceph/ioctl.c +++ b/fs/ceph/ioctl.c @@ -42,6 +42,7 @@ static long __validate_layout(struct ceph_mds_client *mdsc, /* validate striping parameters */ if ((l-object_size ~PAGE_MASK) || (l-stripe_unit ~PAGE_MASK) || + (l-stripe_unit == 0) || ((unsigned)l-object_size % (unsigned)l-stripe_unit)) return -EINVAL; -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch -next] libceph: fix NULL dereference in reset_connection()
We dereference con-in_msg on the line after it was set to NULL.
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 5e9f61d..6aa671c 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -437,10 +437,10 @@ static void reset_connection(struct ceph_connection *con)
if (con-in_msg) {
BUG_ON(con-in_msg-con != con);
+ ceph_con_put(con-in_msg-con);
con-in_msg-con = NULL;
ceph_msg_put(con-in_msg);
con-in_msg = NULL;
- ceph_con_put(con-in_msg-con);
}
con-connect_seq = 0;
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch -next] libceph: fix NULL dereference in reset_connection()
On Tue, Jun 19, 2012 at 08:27:19AM -0500, Alex Elder wrote: On 06/19/2012 05:33 AM, Dan Carpenter wrote: We dereference con-in_msg on the line after it was set to NULL. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com Yikes. Actually I think I prefer a different fix, which is simply to call ceph_con_put(con) in the same spot it was called with con-in_msg-con before. I'd rather drop the message reference before dropping the connection reference. I.e.: @@ -440,7 +440,7 @@ static void reset_connection(struct ceph_connection *con) con-in_msg-con = NULL; ceph_msg_put(con-in_msg); con-in_msg = NULL; - ceph_con_put(con-in_msg-con); + ceph_con_put(con); } con-connect_seq = 0; (I crafted that manually--it may not work...) I will re-post that fix and will credit you with it. Please acknowledge it's OK with you though. Thanks a lot. Yep. We already know that con-in_msg-con and con are the same from the BUG_ON() so this works. Thanks. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] rbd: endian bug in rbd_req_cb()
Sparse complains about this because: drivers/block/rbd.c:996:20: warning: cast to restricted __le32 drivers/block/rbd.c:996:20: warning: cast from restricted __le16 These are set in osd_req_encode_op() and they are le16. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index c1f7701..64d3d6f 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -993,7 +993,7 @@ static void rbd_req_cb(struct ceph_osd_request *req, struct ceph_msg *msg) op = (void *)(replyhead + 1); rc = le32_to_cpu(replyhead-result); bytes = le64_to_cpu(op-extent.length); - read_op = (le32_to_cpu(op-op) == CEPH_OSD_OP_READ); + read_op = (le16_to_cpu(op-op) == CEPH_OSD_OP_READ); dout(rbd_req_cb bytes=%lld readop=%d rc=%d\n, bytes, read_op, rc); -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: ceph: move encode_fh to new API
Hello Sage Weil,
This is a semi-automatic email about new static checker warnings.
The patch f59919a07e03: ceph: move encode_fh to new API from Apr 5,
2012, leads to the following Smatch complaint:
fs/ceph/export.c:85 ceph_encode_fh()
error: we previously assumed 'dentry' could be null (see line 67)
fs/ceph/export.c
66 /* if we found an alias, generate a connectable fh */
67 if (*max_len = connected_handle_length dentry) {
^^
New check.
68 dout(encode_fh %p connectable\n, dentry);
69 spin_lock(dentry-d_lock);
70 parent = dentry-d_parent;
71 cfh-ino = ceph_ino(inode);
72 cfh-parent_ino = ceph_ino(parent-d_inode);
73 cfh-parent_name_hash =
ceph_dentry_hash(parent-d_inode,
74 dentry);
75 *max_len = connected_handle_length;
76 type = 2;
77 spin_unlock(dentry-d_lock);
78 } else if (*max_len = handle_length) {
79 if (parent_inode) {
80 /* nfsd wants connectable */
81 *max_len = connected_handle_length;
82 type = 255;
83 } else {
84 dout(encode_fh %p\n, dentry);
85 fh-ino = ceph_ino(dentry-d_inode);
^^^
Old dereference.
86 *max_len = handle_length;
87 type = 1;
These emails really are mostly automated... So if it's a false positive
then I blame the script. Hope it's not too much spam.
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] rbd: use gfp_flags parameter in rbd_header_from_disk()
We should use the gfp_flags that the caller specified instead of
GFP_KERNEL here.
There is only one caller and it uses GFP_KERNEL, so this change is just
a cleanup and doesn't change how the code works.
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a6278e7..fc9341f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -474,11 +474,11 @@ static int rbd_header_from_disk(struct rbd_image_header
*header,
return -ENOMEM;
if (snap_count) {
header-snap_names = kmalloc(header-snap_names_len,
-GFP_KERNEL);
+gfp_flags);
if (!header-snap_names)
goto err_snapc;
header-snap_sizes = kmalloc(snap_count * sizeof(u64),
-GFP_KERNEL);
+gfp_flags);
if (!header-snap_sizes)
goto err_names;
} else {
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 1/2] ceph: null deref on allocation failure
The original code checked event_work for allocation failures, but only
after it had already use it.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 02212ed..b6776cb 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1602,11 +1602,11 @@ void handle_watch_notify(struct ceph_osd_client *osdc,
struct ceph_msg *msg)
cookie, ver, event);
if (event) {
event_work = kmalloc(sizeof(*event_work), GFP_NOIO);
- INIT_WORK(event_work-work, do_event_work);
if (!event_work) {
dout(ERROR: could not allocate event_work\n);
goto done_err;
}
+ INIT_WORK(event_work-work, do_event_work);
event_work-event = event;
event_work-ver = ver;
event_work-notify_id = notify_id;
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
potential null dereference in __dcache_readdir()
Hi hi!
This is a smatch thing. We check if last is NULL and then dereference
it later with out checking. It might be worth looking at. I'm not
familiar enough with the code to know the fix.
It comes from:
commit 2817b000b02c5f0c05af67c01fb2684e1381d6ef
Author: Sage Weil s...@newdream.net
Date: Tue Oct 6 11:31:08 2009 -0700
ceph: directory operations
regards,
dan carpenter
fs/ceph/dir.c +124 __dcache_readdir(28) error: we previously assumed 'last'
could be null.
116 /* start at beginning? */
117 if (filp-f_pos == 2 || (last
checked here.
118 filp-f_pos
ceph_dentry(last)-offset)) {
119 if (list_empty(parent-d_subdirs))
120 goto out_unlock;
121 p = parent-d_subdirs.prev;
122 dout( initial p %p/%p\n, p-prev, p-next);
123 } else {
124 p = last-d_u.d_child.prev;
^^
dereferenced here.
125 }
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 1/2 -next] rbd: null vs ERR_PTR
ceph_alloc_page_vector() returns ERR_PTR(-ENOMEM) on errors.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1ac87f1..52f9420 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -826,8 +826,8 @@ static int rbd_req_sync_op(struct rbd_device *dev,
num_pages = calc_pages_for(ofs , len);
pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
- if (!pages)
- return -ENOMEM;
+ if (IS_ERR(pages))
+ return PTR_ERR(pages);
if (!orig_ops) {
payload_len = (flags CEPH_OSD_FLAG_WRITE ? len : 0);
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 2/2 -next] rbd: passing wrong variable to bvec_kunmap_irq()
We should be passing buf here insead of bv. This is tricky because it's not the same as kmap() and kunmap(). GCC does warn about it if you compile on i386 with CONFIG_HIGHMEM. Signed-off-by: Dan Carpenter erro...@gmail.com --- If it's any consolation, out of three callers to bvec_kunmap_irq() only one caller was correct. :P diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 52f9420..6ec9d53 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -554,7 +554,7 @@ static void zero_bio_chain(struct bio *chain, int start_ofs) buf = bvec_kmap_irq(bv, flags); memset(buf + remainder, 0, bv-bv_len - remainder); - bvec_kunmap_irq(bv, flags); + bvec_kunmap_irq(buf, flags); } pos += bv-bv_len; } -- To unsubscribe from this list: send the line unsubscribe ceph-devel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] ceph: ceph_mdsc_build_path() returns an ERR_PTR
ceph_mdsc_build_path() returns an ERR_PTR but this code is set up to
handle NULL returns.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 360c4f2..6fd8b20 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -171,6 +171,8 @@ static int mdsc_show(struct seq_file *s, void *p)
} else if (req-r_dentry) {
path = ceph_mdsc_build_path(req-r_dentry, pathlen,
pathbase, 0);
+ if (IS_ERR(path))
+ path = NULL;
spin_lock(req-r_dentry-d_lock);
seq_printf(s, #%llx/%.*s (%s),
ceph_ino(req-r_dentry-d_parent-d_inode),
@@ -187,6 +189,8 @@ static int mdsc_show(struct seq_file *s, void *p)
if (req-r_old_dentry) {
path = ceph_mdsc_build_path(req-r_old_dentry, pathlen,
pathbase, 0);
+ if (IS_ERR(path))
+ path = NULL;
spin_lock(req-r_old_dentry-d_lock);
seq_printf(s, #%llx/%.*s (%s),
ceph_ino(req-r_old_dentry-d_parent-d_inode),
--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
