On Sat, 30 Nov 2013 17:02:26 +0000 Al Viro <[email protected]> wrote:
> BTW, what happens if svc_export_request() ends up with pathname filling
> almost all space left, so that qword_add(bpp, blen, pth) right after the
> call of d_path() in there overwrites the beginning of d_path() output?
> Neil? And while we are at it, handling of overflow in there looks also
> looks fishy...
In this case the returned *blen will be negative so cache_request() in
net/sunrpc/cache.h will return -EAGAIN.
cache_read() would then go into a tight loop, trying again and again to
create the request, but it will never fit in the buffer.
I guess maybe an EINVAL might help there, plus code to skip over impossible
requests.
Maybe the following. We'd need to double check that no ->cache_request
function can fail in a way that is worth retrying, but I doubt they would.
Any thoughts Bruce?
Thanks,
NeilBrown
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index a72de074172d..a065f827e2a3 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -748,7 +748,7 @@ static int cache_request(struct cache_detail *detail,
detail->cache_request(detail, crq->item, &bp, &len);
if (len < 0)
- return -EAGAIN;
+ return -EINVAL;
return PAGE_SIZE - len;
}
@@ -788,6 +788,11 @@ static ssize_t cache_read(struct file *filp, char __user *buf, size_t count,
if (rq->len == 0) {
err = cache_request(cd, rq);
+ if (err == -EINVAL) {
+ spin_lock(&queue_lock);
+ list_move(&rp->q.list, &rq->q.list);
+ spin_unlock(&queue_lock);
+ }
if (err < 0)
goto out;
rq->len = err;
On Mon, Dec 02, 2013 at 04:43:59PM +1100, NeilBrown wrote:
> On Sat, 30 Nov 2013 17:02:26 +0000 Al Viro <[email protected]> wrote:
>
>
> > BTW, what happens if svc_export_request() ends up with pathname filling
> > almost all space left, so that qword_add(bpp, blen, pth) right after the
> > call of d_path() in there overwrites the beginning of d_path() output?
> > Neil? And while we are at it, handling of overflow in there looks also
> > looks fishy...
>
> In this case the returned *blen will be negative so cache_request() in
> net/sunrpc/cache.h will return -EAGAIN.
> cache_read() would then go into a tight loop, trying again and again to
> create the request, but it will never fit in the buffer.
>
> I guess maybe an EINVAL might help there, plus code to skip over impossible
> requests.
> Maybe the following. We'd need to double check that no ->cache_request
> function can fail in a way that is worth retrying, but I doubt they would.
>
> Any thoughts Bruce?
Yes, the cache_request failures are all similarly fatal overflows.
Uh, not sure if I remember how the data structures here work: so
userspace is now going to get an -EINVAL on read, and may just end up
retrying the read?
Or do we hit the "need to release rq" case in cache_read and just
discard the request?
--b.
>
> Thanks,
> NeilBrown
>
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index a72de074172d..a065f827e2a3 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -748,7 +748,7 @@ static int cache_request(struct cache_detail *detail,
>
> detail->cache_request(detail, crq->item, &bp, &len);
> if (len < 0)
> - return -EAGAIN;
> + return -EINVAL;
> return PAGE_SIZE - len;
> }
>
> @@ -788,6 +788,11 @@ static ssize_t cache_read(struct file *filp, char __user *buf, size_t count,
>
> if (rq->len == 0) {
> err = cache_request(cd, rq);
> + if (err == -EINVAL) {
> + spin_lock(&queue_lock);
> + list_move(&rp->q.list, &rq->q.list);
> + spin_unlock(&queue_lock);
> + }
> if (err < 0)
> goto out;
> rq->len = err;