linux-kernel - RE: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 8 Mar 2014 10:27:54 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	David Miller <davem@...emloft.net>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>
Subject: RE: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O



> -----Original Message-----
> From: David Miller [mailto:davem@...emloft.net]
> Sent: Saturday, March 8, 2014 12:07 PM
> To: KY Srinivasan
> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; olaf@...fle.de; apw@...onical.com;
> jasowang@...hat.com
> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
> 
> From: KY Srinivasan <kys@...rosoft.com>
> Date: Sat, 8 Mar 2014 04:12:01 +0000
> 
> >
> >
> >> -----Original Message-----
> >> From: David Miller [mailto:davem@...emloft.net]
> >> Sent: Saturday, March 8, 2014 3:18 AM
> >> To: KY Srinivasan
> >> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> >> devel@...uxdriverproject.org; olaf@...fle.de; apw@...onical.com;
> >> jasowang@...hat.com
> >> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter
> >> gather I/O
> >>
> >> From: "K. Y. Srinivasan" <kys@...rosoft.com>
> >> Date: Thu,  6 Mar 2014 21:32:36 -0800
> >>
> >> > +static u32 fill_pg_buf(struct page *page, u32 offset, u32 len,
> >> > +			struct hv_page_buffer *pb)
> >> > +{
> >> > +	int j = 0;
> >> > +
> >> > +	/* Deal with compund pages by ignoring unused part
> >> > +	 * of the page.
> >> > +	 */
> >> > +	page += offset  >> PAGE_SHIFT;
> >>
> >> Please only one space between "offset" and ">>"
> >>
> >> > +	offset &= ~PAGE_MASK;
> >> > +
> >> > +	while (len > 0) {
> >> > +		unsigned long bytes;
> >> > +
> >> > +		bytes = PAGE_SIZE - offset;
> >> > +		if (bytes > len)
> >> > +			bytes = len;
> >> > +		pb[j].pfn = page_to_pfn(page);
> >> > +		pb[j].offset = offset;
> >> > +		pb[j].len = bytes;
> >> > +
> >> > +		offset += bytes;
> >> > +		len -= bytes;
> >> > +
> >> > +		if (offset == PAGE_SIZE && len) {
> >> > +			page++;
> >> > +			offset = 0;
> >> > +			j++;
> >> > +		}
> >> > +	}
> >> > +
> >> > +	return j + 1;
> >> > +}
> >>
> >> I think this function has some edge case errors.
> >>
> >> As I understand it, this function returns how many page buffer
> >> entries were filled in.
> >>
> >> But if we fill exactly the end of a page, we will report one too many
> >> in the return value.
> >
> > I may be missing something here; but in the case you are describing we
> > would return a value of one as we should. When offset + len is exactly
> > equal to the page size, we would not increment the value of j, since
> > at that point len == 0. Since the initial value of j was 0 and we return (j + 1),
> we will return 1.
> 
> That's not what happens, you execute the loop code at least once, and
> hit:
> 
> >> > +		offset += bytes;
> >> > +		len -= bytes;
> >> > +		if (offset == PAGE_SIZE && len) {
> >> > +			page++;
> >> > +			offset = 0;
> >> > +			j++;
> >> > +		}

> 
> thus incrementing j before the return statement.

I don't mean to be argumentative, but I still don't see the issue here. You are right, I
will execute the loop code at least once, but j gets incremented only if len is non-zero as well as
offset being == PAGE_SIZE. "len" would have gotten decremented prior to the check and in the case
we are talking about, "len" would be zero and so j would not get incremented.

Regards,

K. Y

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/