* [PATCH] net: qrtr: fix memory leak in qrtr_tun_read_iter
@ 2019-09-25 23:04 Navid Emamdoost
2019-09-25 23:21 ` Al Viro
0 siblings, 1 reply; 3+ messages in thread
From: Navid Emamdoost @ 2019-09-25 23:04 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, David S. Miller,
netdev, linux-kernel
In qrtr_tun_read_iter we need an error handling path to appropriately
release skb in cases of error.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
net/qrtr/tun.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
index e35869e81766..0f6e6d1d2901 100644
--- a/net/qrtr/tun.c
+++ b/net/qrtr/tun.c
@@ -54,19 +54,24 @@ static ssize_t qrtr_tun_read_iter(struct kiocb *iocb, struct iov_iter *to)
int count;
while (!(skb = skb_dequeue(&tun->queue))) {
- if (filp->f_flags & O_NONBLOCK)
- return -EAGAIN;
+ if (filp->f_flags & O_NONBLOCK) {
+ count = -EAGAIN;
+ goto out;
+ }
/* Wait until we get data or the endpoint goes away */
if (wait_event_interruptible(tun->readq,
- !skb_queue_empty(&tun->queue)))
- return -ERESTARTSYS;
+ !skb_queue_empty(&tun->queue))) {
+ count = -ERESTARTSYS;
+ goto out;
+ }
}
count = min_t(size_t, iov_iter_count(to), skb->len);
if (copy_to_iter(skb->data, count, to) != count)
count = -EFAULT;
+out:
kfree_skb(skb);
return count;
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net: qrtr: fix memory leak in qrtr_tun_read_iter
2019-09-25 23:04 [PATCH] net: qrtr: fix memory leak in qrtr_tun_read_iter Navid Emamdoost
@ 2019-09-25 23:21 ` Al Viro
2019-09-27 3:16 ` Navid Emamdoost
0 siblings, 1 reply; 3+ messages in thread
From: Al Viro @ 2019-09-25 23:21 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, smccaman, kjlu, David S. Miller, netdev, linux-kernel
On Wed, Sep 25, 2019 at 06:04:13PM -0500, Navid Emamdoost wrote:
> In qrtr_tun_read_iter we need an error handling path to appropriately
> release skb in cases of error.
Release _what_ skb?
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> net/qrtr/tun.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
> index e35869e81766..0f6e6d1d2901 100644
> --- a/net/qrtr/tun.c
> +++ b/net/qrtr/tun.c
> @@ -54,19 +54,24 @@ static ssize_t qrtr_tun_read_iter(struct kiocb *iocb, struct iov_iter *to)
> int count;
>
> while (!(skb = skb_dequeue(&tun->queue))) {
The body of the loop is entered only if the loop condition has
evaluated true. In this case, it means that the value of
!(skb = skb_dequeue(&tun->queue))
had been true, i.e. the value of
skb = skb_dequeue(&tun->queue)
has been NULL, i.e. that skb_dequeue() has returned NULL, which had
been copied into skb.
In other words, in the body of that loop we have skb equal to NULL.
> - if (filp->f_flags & O_NONBLOCK)
> - return -EAGAIN;
> + if (filp->f_flags & O_NONBLOCK) {
> + count = -EAGAIN;
> + goto out;
> + }
>
> /* Wait until we get data or the endpoint goes away */
> if (wait_event_interruptible(tun->readq,
> - !skb_queue_empty(&tun->queue)))
> - return -ERESTARTSYS;
> + !skb_queue_empty(&tun->queue))) {
> + count = -ERESTARTSYS;
> + goto out;
> + }
> }
The meaning of that loop is fairly clear, isn't it? Keep looking int
tun->queue until an skb shows up there. If it's not immediately there,
fail with -EAGAIN for non-blocking files and wait on tun->readq until
some skb arrives.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net: qrtr: fix memory leak in qrtr_tun_read_iter
2019-09-25 23:21 ` Al Viro
@ 2019-09-27 3:16 ` Navid Emamdoost
0 siblings, 0 replies; 3+ messages in thread
From: Navid Emamdoost @ 2019-09-27 3:16 UTC (permalink / raw)
To: Al Viro; +Cc: emamd001, smccaman, kjlu, David S. Miller, netdev, linux-kernel
On Thu, Sep 26, 2019 at 12:21:12AM +0100, Al Viro wrote:
> On Wed, Sep 25, 2019 at 06:04:13PM -0500, Navid Emamdoost wrote:
> > In qrtr_tun_read_iter we need an error handling path to appropriately
> > release skb in cases of error.
>
> Release _what_ skb?
It is not a leak clearly! My bad ...
>
> > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> > ---
> > net/qrtr/tun.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
> > index e35869e81766..0f6e6d1d2901 100644
> > --- a/net/qrtr/tun.c
> > +++ b/net/qrtr/tun.c
> > @@ -54,19 +54,24 @@ static ssize_t qrtr_tun_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > int count;
> >
> > while (!(skb = skb_dequeue(&tun->queue))) {
>
> The body of the loop is entered only if the loop condition has
> evaluated true. In this case, it means that the value of
> !(skb = skb_dequeue(&tun->queue))
> had been true, i.e. the value of
> skb = skb_dequeue(&tun->queue)
> has been NULL, i.e. that skb_dequeue() has returned NULL, which had
> been copied into skb.
>
> In other words, in the body of that loop we have skb equal to NULL.
>
> > - if (filp->f_flags & O_NONBLOCK)
> > - return -EAGAIN;
> > + if (filp->f_flags & O_NONBLOCK) {
> > + count = -EAGAIN;
> > + goto out;
> > + }
> >
> > /* Wait until we get data or the endpoint goes away */
> > if (wait_event_interruptible(tun->readq,
> > - !skb_queue_empty(&tun->queue)))
> > - return -ERESTARTSYS;
> > + !skb_queue_empty(&tun->queue))) {
> > + count = -ERESTARTSYS;
> > + goto out;
> > + }
> > }
>
> The meaning of that loop is fairly clear, isn't it? Keep looking int
> tun->queue until an skb shows up there. If it's not immediately there,
> fail with -EAGAIN for non-blocking files and wait on tun->readq until
> some skb arrives.
Thanks for the explainations.
Navid.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-27 3:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 23:04 [PATCH] net: qrtr: fix memory leak in qrtr_tun_read_iter Navid Emamdoost
2019-09-25 23:21 ` Al Viro
2019-09-27 3:16 ` Navid Emamdoost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).