| Message ID | 20170710130833.1834210-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote: > Thanks for testing it! > > That means we did not copy any data and the kernel continues with > an uninitialized buffer, right? The problem may be the definition of > > struct kib_immediate_msg { > struct lnet_hdr ibim_hdr; /* portals header */ > char ibim_payload[0]; /* piggy-backed payload */ > } WIRE_ATTR; > > The check that Al added will try to ensure that we don't write > beyond the size of the ibim_payload[] array, which unfortunately > is defined as a zero-byte array, so I can see why it will now > fail. However, it's already broken in mainline now, with or without > my patch. > > Are you able to come up with a fix that avoids the warning in > 'allmodconfig' and makes the function do something reasonable > again? Might make sense to try and use valid C99 for "array of indefinite size as the last member", i.e. struct kib_immediate_msg { struct lnet_hdr ibim_hdr; /* portals header */ char ibim_payload[]; /* piggy-backed payload */ } WIRE_ATTR; Zero-sized array as the last member is gcc hack predating that; looks like gcc gets confused into deciding that it knows the distance from the end of object... Said that, are we really guaranteed the IBLND_MSG_SIZE bytes in there?
On Fri, 14 Jul 2017, Al Viro wrote: > On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote: > > > Thanks for testing it! > > > > That means we did not copy any data and the kernel continues with > > an uninitialized buffer, right? The problem may be the definition of > > > > struct kib_immediate_msg { > > struct lnet_hdr ibim_hdr; /* portals header */ > > char ibim_payload[0]; /* piggy-backed payload */ > > } WIRE_ATTR; > > > > The check that Al added will try to ensure that we don't write > > beyond the size of the ibim_payload[] array, which unfortunately > > is defined as a zero-byte array, so I can see why it will now > > fail. However, it's already broken in mainline now, with or without > > my patch. > > > > Are you able to come up with a fix that avoids the warning in > > 'allmodconfig' and makes the function do something reasonable > > again? Yes, I'm testing a fix right now which I will merge with the original patch. Greg this patch will need to be sent to Linus as well so the kernel release isn't broken for users. > Might make sense to try and use valid C99 for "array of indefinite > size as the last member", i.e. > struct kib_immediate_msg { > struct lnet_hdr ibim_hdr; /* portals header */ > char ibim_payload[]; /* piggy-backed payload */ > } WIRE_ATTR; > > Zero-sized array as the last member is gcc hack predating that; > looks like gcc gets confused into deciding that it knows the distance > from the end of object... I did some profiling and found gcc was doing the right thing. That should be updated to a C99 flexable array in a latter patch. > Said that, are we really guaranteed the IBLND_MSG_SIZE bytes > in there? This is what the real bug was. In the current code we are telling copy_from_iter and copy_to_iter that the number of bytes are always IBLND_MSG_SIZE. Arnd thought this was always the size so in his patch he was testing the returned result of copy_[from|to]_iter to IBLND_MSG_SIZE. This nearly always failed since variable sized messages are being created. The zero size I initially saw was from doing pings. When I later tested with pushing I/O packets of other sizes were observed but none of them were IBLND_MSG_SIZE in size so they failed to transmit. As soon as I'm done testing I will send a patch.
diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c index 85b242ec5f9b..70256a0ffd2e 100644 --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c @@ -1640,8 +1640,13 @@ kiblnd_send(struct lnet_ni *ni, void *private, struct lnet_msg *lntmsg) ibmsg = tx->tx_msg; ibmsg->ibm_u.immediate.ibim_hdr = *hdr; - copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE, + rc = copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE, &from); + if (rc != IBLND_MSG_SIZE) { + kiblnd_pool_free_node(&tx->tx_pool->tpo_pool, &tx->tx_list); + return -EFAULT; + } + nob = offsetof(struct kib_immediate_msg, ibim_payload[payload_nob]); kiblnd_init_tx_msg(ni, tx, IBLND_MSG_IMMEDIATE, nob); @@ -1741,8 +1746,14 @@ kiblnd_recv(struct lnet_ni *ni, void *private, struct lnet_msg *lntmsg, break; } - copy_to_iter(&rxmsg->ibm_u.immediate.ibim_payload, + rc = copy_to_iter(&rxmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE, to); + if (rc != IBLND_MSG_SIZE) { + rc = -EFAULT; + break; + } + + rc = 0; lnet_finalize(ni, lntmsg, 0); break;
We now get a helpful warning for code that calls copy_{from,to}_iter without checking the return value, introduced by commit aa28de275a24 ("iov_iter/hardening: move object size checks to inlined part"). drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function 'kiblnd_send': drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1643:2: error: ignoring return value of 'copy_from_iter', declared with attribute warn_unused_result [-Werror=unused-result] drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function 'kiblnd_recv': drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1744:3: error: ignoring return value of 'copy_to_iter', declared with attribute warn_unused_result [-Werror=unused-result] In case we get short copies here, we may get incorrect behavior. I've added failure handling for both rx and tx now, returning -EFAULT as expected. Cc: stable@vger.kernel.org Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- This warning now shows up in 'allmodconfig' builds, so it would be good to get it fixed quickly for 4.13, but my patch should not go in without careful review since I'm not familiar with with code and the error handling is a bit tricky here. I added 'Cc: stable' since this is a preexisting condition that we only started warning about now. --- drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) -- 2.9.0