[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Piotr Ziecik]

When packets arrive on the interface that are larger than the buffer
being passed to U-Boot from a standalone application, then the eth_receive()
returns -1 and leaves the packet saved. The next call to eth_receive()
will find that same packet and can fail for the exact same reason.
A typical scenario is the loader doing ARP with a buffer of 66 bytes.
The end result is that the ARP will fail and the loader panics.

This patch fixes above problem by allowing partial packet read.

Signed-off-by: Marcel Moolenaar <xcllnt@mac.com>
Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
---
 net/eth.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 3d93966..f0124f8 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -416,10 +416,7 @@ int eth_receive(volatile void *packet, int length)
 			return -1;
 	}
 
-	if (length < eth_rcv_bufs[eth_rcv_current].length)
-		return -1;
-
-	length = eth_rcv_bufs[eth_rcv_current].length;
+	length = min(length, eth_rcv_bufs[eth_rcv_current].length);
 
 	for (i = 0; i < length; i++)
 		p[i] = eth_rcv_bufs[eth_rcv_current].data[i];
-- 
1.5.2.2

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Mike Frysinger]

On Tuesday 14 July 2009 04:08:35 Piotr Ziecik wrote:
> When packets arrive on the interface that are larger than the buffer
> being passed to U-Boot from a standalone application, then the
> eth_receive() returns -1 and leaves the packet saved. The next call to
> eth_receive() will find that same packet and can fail for the exact same
> reason. A typical scenario is the loader doing ARP with a buffer of 66
> bytes. The end result is that the ARP will fail and the loader panics.
>
> This patch fixes above problem by allowing partial packet read.

seems like it could easily introduce incorrect behavior in existing 
applications.  the code also sounds a bit risky ... your change would mean 
people could read the leading part, but the rest is lost ?

probably better to add a new function with explicit semantics -- 
eth_receive_partial() or something.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090715/1fc7cdfc/attachment.pgp 

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Piotr Zięcik]

Wednesday 15 July 2009 17:05:44 Mike Frysinger napisa?(a):
> On Tuesday 14 July 2009 04:08:35 Piotr Ziecik wrote:
> > When packets arrive on the interface that are larger than the buffer
> > being passed to U-Boot from a standalone application, then the
> > eth_receive() returns -1 and leaves the packet saved. The next call to
> > eth_receive() will find that same packet and can fail for the exact same
> > reason. A typical scenario is the loader doing ARP with a buffer of 66
> > bytes. The end result is that the ARP will fail and the loader panics.
> >
> > This patch fixes above problem by allowing partial packet read.
>
> seems like it could easily introduce incorrect behavior in existing
> applications.  the code also sounds a bit risky ... your change would mean
> people could read the leading part, but the rest is lost ?

Yes. The patch allows read only first n bytes from packet. The rest is 
discaded, if buffer is too small. This behaviour is similar to Linux recv() 
function. I do not see why we have to force application to prepare 1,5kB 
buffer for received packets when for example it waits for ARP reply.

> probably better to add a new function with explicit semantics --
> eth_receive_partial() or something.

API uses simple open, read/write, close model. The eth_receive() function is 
used by API read() call on network device. I do not see space here another 
function.

-- 
Best regards,
Piotr Ziecik

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Wolfgang Denk]

Dear Piotr =?iso-8859-2?q?Zi=EAcik?=,

In message <200907161151.59353.kosmo@semihalf.com> you wrote:
>
> > > This patch fixes above problem by allowing partial packet read.
> >
> > seems like it could easily introduce incorrect behavior in existing
> > applications.  the code also sounds a bit risky ... your change would mean
> > people could read the leading part, but the rest is lost ?
> 
> discaded, if buffer is too small. This behaviour is similar to Linux recv()

But recv() is on another level. Here we are dealing with receiving raw
ethernet frames.

> function. I do not see why we have to force application to prepare 1,5kB
> buffer for received packets when for example it waits for ARP reply.

Come on - what exactly is the extra effort you have to spend to
prepare a bigger buffer?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Pig: An animal (Porcus omnivorous) closely allied to the  human  race
by  the splendor and vivacity of its appetite, which, however, is in-
ferior in scope, for it balks at pig.                - Ambrose Bierce

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Piotr Zięcik]

Thursday 16 July 2009 14:39:59 Wolfgang Denk napisa?(a):
>
> But recv() is on another level. Here we are dealing with receiving raw
> ethernet frames.
>

I think that there is an analogy. Both function could be used for receiving 
_frames_. eth_receive() for ethernet frames and Linux recv() for UDP frames.

> > function. I do not see why we have to force application to prepare 1,5kB
> > buffer for received packets when for example it waits for ARP reply.
>
> Come on - what exactly is the extra effort you have to spend to
> prepare a bigger buffer?
>

Maybe I have used bad example. When application prepares a buffer
and chooses it's size to be less than interface MTU it means that application 
is not able to process larger packets. U-Boot should not expect that simple
application using API is able to handle every possible case (for example
9k packets on gigabyte ethernet).

-- 
Pozdrawiam.
Piotr Zi?cik

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Marcel Moolenaar]

On Jul 16, 2009, at 5:39 AM, Wolfgang Denk wrote:

> Dear Piotr =?iso-8859-2?q?Zi=EAcik?=,
>
> In message <200907161151.59353.kosmo@semihalf.com> you wrote:
>>
>>>> This patch fixes above problem by allowing partial packet read.
>>>
>>> seems like it could easily introduce incorrect behavior in existing
>>> applications.  the code also sounds a bit risky ... your change  
>>> would mean
>>> people could read the leading part, but the rest is lost ?
>>
>> discaded, if buffer is too small. This behaviour is similar to  
>> Linux recv()
>
> But recv() is on another level. Here we are dealing with receiving raw
> ethernet frames.

Yes. As such truncation and other protocol errors need
to be checked for. Including the reception of packets
you're not waiting for.

If an application prepares a 100-byte buffer, then it does
so under the assumption that what it's waiting for is not
larger than 100 bytes. This is a fair assumption, because
applications wait for specific packets. In the trigger case:
an ARP response.

This is also the crux of the problem. The application
waits for a specific response, but there's no guarantee
(due to broadcast or multicast packets on the LAN) that
the next packet to arrive on the interface is the one
that we're waiting for.

Now, one approach would be to ignore packets that don't
fit the buffer. This seems unflexible, because it makes
it impossible to employ flexible buffer allocation in
the application. What's left? The only thing left is that
you return whatever arrived on the interface truncated
to the buffer size. That way the application can discard
and call read again if headers don't match, or it can
allocate a bigger buffer and retry.

>
>> function. I do not see why we have to force application to prepare  
>> 1,5kB
>> buffer for received packets when for example it waits for ARP reply.
>
> Come on - what exactly is the extra effort you have to spend to
> prepare a bigger buffer?

The problem with this approach is that theoretically
an application needs to use a buffer that is as large
as the maximum size of a packet that can appear on the
interface. For example, with jumbo frames this means
that any application, module or function that wants to
receive even the smallest amount of data (say an ARP
response) needs to allocate a 9K buffer.

The question is not of effort -- there's virtually none.
The question is whether this is good engineering. Worst
case buffer allocation doesn't strike me as portable nor
reasonable.

My $0.02.

-- 
Marcel Moolenaar
xcllnt at mac.com

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Wolfgang Denk]

Dear Marcel Moolenaar,

In message <CAC78579-738B-4D03-8EF0-14FA1141FAE9@mac.com> you wrote:
> 
> This is also the crux of the problem. The application
> waits for a specific response, but there's no guarantee
> (due to broadcast or multicast packets on the LAN) that
> the next packet to arrive on the interface is the one
> that we're waiting for.

Right.

> Now, one approach would be to ignore packets that don't
> fit the buffer. This seems unflexible, because it makes
> it impossible to employ flexible buffer allocation in
> the application. What's left? The only thing left is that

"flexible buffer allocation"? May I ask what sort of applications you
have in mind? We're talking about applications running in the context
of a boot loader, right? For anything fancy you should probably rather
use an operating system.

> you return whatever arrived on the interface truncated
> to the buffer size. That way the application can discard
> and call read again if headers don't match, or it can
> allocate a bigger buffer and retry.

Allocate a bigger buffer? What for? The packet has been dropped
already and is not recoverable.

> The problem with this approach is that theoretically
> an application needs to use a buffer that is as large
> as the maximum size of a packet that can appear on the
> interface. For example, with jumbo frames this means
> that any application, module or function that wants to
> receive even the smallest amount of data (say an ARP
> response) needs to allocate a 9K buffer.

Right.

> The question is not of effort -- there's virtually none.
> The question is whether this is good engineering. Worst
> case buffer allocation doesn't strike me as portable nor
> reasonable.

Not portable? In which way do you see any porting  issues  here?  Not
reasonable?  How many such buffers do you need, then? All it takes is
a different size  in  simple  call  to  malloc(),  or  am  I  missing
something?

Please let's keep in mind: this is a boot loader, and a very  minimal
network  stack.  It  is  not an OS, and we don't claim to implement a
TCP/IP stack.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We shall reach greater and greater platitudes of achievement."
- Richard J. Daley

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Marcel Moolenaar]

On Jul 16, 2009, at 8:39 AM, Wolfgang Denk wrote:
>> Now, one approach would be to ignore packets that don't
>> fit the buffer. This seems unflexible, because it makes
>> it impossible to employ flexible buffer allocation in
>> the application. What's left? The only thing left is that
>
> "flexible buffer allocation"? May I ask what sort of applications you
> have in mind? We're talking about applications running in the context
> of a boot loader, right? For anything fancy you should probably rather
> use an operating system.

Nothing particular. It was just a random thought that
popped in my head while I was typing. I very much doubt
that anything will be implemented, but it does illustrate
(at least to me :-) how one approach makes assumptions
and thereby eliminating a class of implementations (this
being the "ignore packet" behaviour) while the other
does not, allows that same class of applications, but
does so at the "cost" of having applications check more.

Since we are talking about raw packets, the application
(in this case a boot loader) must check anyway, so the
cost is nihil.

>> you return whatever arrived on the interface truncated
>> to the buffer size. That way the application can discard
>> and call read again if headers don't match, or it can
>> allocate a bigger buffer and retry.
>
> Allocate a bigger buffer? What for? The packet has been dropped
> already and is not recoverable.

The scenario in question is when the truncated packet is
passed to the application. There's no dropping. Depending
on the application it surely can wait for the retransmit
or it can ask for one. Both cases (wait or ask) can be
considered retries...

>> The question is not of effort -- there's virtually none.
>> The question is whether this is good engineering. Worst
>> case buffer allocation doesn't strike me as portable nor
>> reasonable.
>
> Not portable? In which way do you see any porting  issues  here?  Not
> reasonable?  How many such buffers do you need, then? All it takes is
> a different size  in  simple  call  to  malloc(),  or  am  I  missing
> something?

Wolfgang,

It seems to me that these questions stem from an assumption
about how applications are written. That is, I always interpret
these questions as an inquiry into the use of an API so as to
argue about how an API is used, rather than how the API should
behave. Personally, I don't think this is the right approach
when discussing an API, because it the API comes with an assumed
use case.

Anyway: I'll answer your question given above beneath the
following...

> Please let's keep in mind: this is a boot loader, and a very  minimal
> network  stack.  It  is  not an OS, and we don't claim to implement a
> TCP/IP stack.

The application in question is a boot loader. One that does not
implement a TCP/IP stack either. I would argue that it's exactly
the application one would expect to run under U-Boot.

As to the porting issues you were asking about: the FreeBSD boot
loader runs in many environments: PC BIOS, EFI, Open Firmware,
U-Boot, ARC (obsolete), etc.
If The U-Boot APIs requires the FreeBSD loader to allocate 9K
buffers just so that it can receive a tiny ARP response, then
it's the only environment to do so. Changing the FreeBSD loader
to work with U-Boot affects the loader in the other environments
as well. This by itself shows that there's a portability issue.

As for the number of buffers: we need 1 for ARP. A small one I
might add. However, BOOTP/DHCP also needs a buffer. That's one
more. TFTP needs a buffer. etc, etc...
The question as to how many buffers we need cannot really be
answered without digressing in "why don't you restructure your
code so that you only need 1". It all depends on how code is
organized, designed, modularized or combined. In the case of
the FreeBSD loader we a few places to change. This does not
include all the non-open features that people have added to the
loader.


Let me try to spin it differently:

The U-Boot APIs were intended and designed to work with any
application. More to the point: they we designed, implemented
and added to U-Boot *because* of the need arising from wanting
to use the FreeBSD loader under U-Boot.
A such, the APIs were designed with the loader in mind and
they were tested with the FreeBSD loader. Besides adding a
U-Boot interface layer to the loader, no loader changes were
made.

Ok, there's bug. Or at least a scenario that wasn't really
thought about or considered. The pivotal application, key
in designing and implementing the APIs, shows that the API
in U-Boot can hang. [The hang being caused by the reception
of a packet that is larger than the buffer the application
provides]. There are 2 simple fixed that would fix the API:
1) just drop the packet. 2) return a truncated packet.

However, I'm currently in a discussion that suggests that
the application should use bigger buffers. That strikes me
as odd, because by intend and design the API was to support
the application without requiring it to use bigger buffers.
Put differently: by requiring the application to use bigger
buffers, the API ipso-facto stops supporting the one app
it was designed to support. Is this a good way to fix an
otherwise minor problem?

To conclude:
I'm happy with an API change, whether it's dropping the
packet or returning a truncated one. Personally I favor
the truncation, because we're dealing with raw packets
and the application is expected to make sure it received
a proper packet to begin with.

Changing the semantics of the API and require all
applications to allocate a bigger buffer to handle this
is not a solution in my opinion.

In any case: the final verdict is with the U-Boot community.
I stated my case and look forward to the resolution so that
I can assess the consequences to me, FreeBSD, as well as my
employer (in reverse order of importance :-)

Cheers,

-- 
Marcel Moolenaar
xcllnt at mac.com

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Ben Warren]

Mike Frysinger wrote:
> On Tuesday 14 July 2009 04:08:35 Piotr Ziecik wrote:
>   
>> When packets arrive on the interface that are larger than the buffer
>> being passed to U-Boot from a standalone application, then the
>> eth_receive() returns -1 and leaves the packet saved. The next call to
>> eth_receive() will find that same packet and can fail for the exact same
>> reason. A typical scenario is the loader doing ARP with a buffer of 66
>> bytes. The end result is that the ARP will fail and the loader panics.
>>
>> This patch fixes above problem by allowing partial packet read.
>>     
>
> seems like it could easily introduce incorrect behavior in existing 
> applications.  the code also sounds a bit risky ... your change would mean 
> people could read the leading part, but the rest is lost ?
>
> probably better to add a new function with explicit semantics -- 
> eth_receive_partial() or something.
> -mike
>   
I've read this whole thread, but this seems the most appropriate place 
to respond. 

I don't like changing the behavior of an existing call to truncation 
from an error, since who knows what will break.  Adding another call 
with different, documented behavior seems more appropriate.  I agree 
with Wolfgang that this really needs to be simple and personally don't 
see the burdens involved in allocating full MTU-sized buffers, but 
obviously can't envision all usage scenarios.

The talk of jumbo frames is currently moot, as the buffer size is fixed 
in U-boot @ 1518 (#define PKTSIZE 1518).  This should probably be 
configurable, but that's another matter.

regards,
Ben

[U-Boot] [PATCH] eth_receive(): Do not assume that caller always wants full packet.

[Wolfgang Denk]

Dear Marcel Moolenaar,

In message <018C5018-F1CE-4E30-9239-93C118CFF386@mac.com> you wrote:
> 
> It seems to me that these questions stem from an assumption
> about how applications are written. That is, I always interpret

This is actually correct. The basic assumption I  make,  and  request
others  to  make too, is that we are working on a boot loader. If you
compare finctionality with other boot  loaders  you  realize  quickly
that  we  are  stretching  this  concept  pretty  far - but there are
certain limits.

Things like more complex network protocols simply have no place in  a
boot loader. If you need something like that, then forget the API for
standalone  applications  and boot an OS instead. It's much faster to
develop, and much more flexible.

> these questions as an inquiry into the use of an API so as to
> argue about how an API is used, rather than how the API should
> behave. Personally, I don't think this is the right approach
> when discussing an API, because it the API comes with an assumed
> use case.

There is an assumed use case: things simple enough to fit into a boot
loader.

> As to the porting issues you were asking about: the FreeBSD boot
> loader runs in many environments: PC BIOS, EFI, Open Firmware,
> U-Boot, ARC (obsolete), etc.

Which of these environments provide an API  as  you  are  asking  for
here?

> As for the number of buffers: we need 1 for ARP. A small one I
> might add. However, BOOTP/DHCP also needs a buffer. That's one
> more. TFTP needs a buffer. etc, etc...

Um... do these have to be separate buffers? Or would a single shared
buffer be sufficient, too?

We already have code in U-Boot that performs ARP, and BOOTP/DHCP, and
TFTP. Why don't you use that code,  instead  of  only  the  low-level
primitives like packet receive?


[Note that I am _not_ trying to just ask provocant questions. I'm
really trying to understand your problem.]

> The question as to how many buffers we need cannot really be
> answered without digressing in "why don't you restructure your
> code so that you only need 1". It all depends on how code is
> organized, designed, modularized or combined. In the case of
> the FreeBSD loader we a few places to change. This does not
> include all the non-open features that people have added to the
> loader.

I must admit that I'm not too eager to add complexity  to  code  that
can  be  simple for U-Boot's own needs just to support some "non-open
features" ...


> The U-Boot APIs were intended and designed to work with any
> application. More to the point: they we designed, implemented

...with any application running in the context and the restrictions of
a boot loader. There has never been any intention to provide a runtime
environment for complex things like an OS.

> Ok, there's bug. Or at least a scenario that wasn't really
> thought about or considered. The pivotal application, key
> in designing and implementing the APIs, shows that the API
> in U-Boot can hang. [The hang being caused by the reception
> of a packet that is larger than the buffer the application
> provides]. There are 2 simple fixed that would fix the API:
> 1) just drop the packet. 2) return a truncated packet.

Agreed. Both will change the behaviour of the current, common network
code. Mike asked if there is any risk involved with  such  a  change,
and if we not better used a new function for this purpose.

> However, I'm currently in a discussion that suggests that
> the application should use bigger buffers. That strikes me
> as odd, because by intend and design the API was to support
> the application without requiring it to use bigger buffers.

We're not talking about design of the API, but about changes  to  the
U-Boot core network code.

> Put differently: by requiring the application to use bigger
> buffers, the API ipso-facto stops supporting the one app
> it was designed to support. Is this a good way to fix an
> otherwise minor problem?

Hm... needing to change the core just  to  satisfy  the  needs  of  a
single  user  with  exotic  requirements  is something I don't decide
quickly. At minimum I want to understand the need for the change, and
be sure that it has no negative impact on other  users  of  the  same
code.

> To conclude:
> I'm happy with an API change, whether it's dropping the
> packet or returning a truncated one. Personally I favor
> the truncation, because we're dealing with raw packets
> and the application is expected to make sure it received
> a proper packet to begin with.
> 
> Changing the semantics of the API and require all
> applications to allocate a bigger buffer to handle this
> is not a solution in my opinion.

You see just your use case, I try to see the  whole  U-Boot  project.