Re: [PATCH 2/2] [NETFILTER] remove bogus hand-coded htonll()
On Sat, Sep 03, 2005 at 09:54:25AM -0700, Jouni Malinen wrote: > On Sat, Sep 03, 2005 at 10:43:15AM +0200, Harald Welte wrote: > > > htonll() is nothing else than cpu_to_be64(), so we'd rather call the > > latter. > > Actually, the htonll() implementation does not seem to be doing what > cpu_to_be64() is doing.. However, I would assume this is a bug in > htonll() and this change to use cpu_to_be64() is fixing that. ACK. > Can this bug cause any major problems in the current implementation? the "current implementation" was only merged after 2.6.13 is released, so I doubt anyone but the netfilter developers is using it yet. > I would assume that the first index should have had '-i' added to it, if > the purpose is to swap byte order.. The code here is leaving some > arbitrary data in 7 bytes of the 64-bit variable and setting > (u8*)[7] = (u8*)[7] in somewhat inefficient way ;-). In addition, > this looks more like swap-8-bytes-unconditionally than doing this based > on host byte order.. yes, yes, yes. Somehow this ancient buggy implementation slipped into mainline. I know I had fixed this before. So please let's all forget about this embarrassing htonll() and move on. -- - Harald Welte <[EMAIL PROTECTED]> http://netfilter.org/ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed."-- Paul Vixie pgpZliKCIUmq8.pgp Description: PGP signature
Re: [PATCH 2/2] [NETFILTER] remove bogus hand-coded htonll()
On Sat, Sep 03, 2005 at 10:43:15AM +0200, Harald Welte wrote:
> htonll() is nothing else than cpu_to_be64(), so we'd rather call the
> latter.
Actually, the htonll() implementation does not seem to be doing what
cpu_to_be64() is doing.. However, I would assume this is a bug in
htonll() and this change to use cpu_to_be64() is fixing that. Can this
bug cause any major problems in the current implementation?
> -u_int64_t htonll(u_int64_t in)
> -{
> - u_int64_t out;
> - int i;
> -
> - for (i = 0; i < sizeof(u_int64_t); i++)
> - ((u_int8_t *))[sizeof(u_int64_t)-1] = ((u_int8_t *))[i];
I would assume that the first index should have had '-i' added to it, if
the purpose is to swap byte order.. The code here is leaving some
arbitrary data in 7 bytes of the 64-bit variable and setting
(u8*)[7] = (u8*)[7] in somewhat inefficient way ;-). In addition,
this looks more like swap-8-bytes-unconditionally than doing this based
on host byte order..
--
Jouni MalinenPGP id EFC895FA
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] [NETFILTER] remove bogus hand-coded htonll()
Hi Dave, please apply the appended patch.
I somehow thought I had fixed this quite some time ago. Probably I lost
it with some merge :(
Thanks,
--
- Harald Welte <[EMAIL PROTECTED]> http://netfilter.org/
"Fragmentation is like classful addressing -- an interesting early
architectural error that shows how much experimentation was going
on while IP was being designed."-- Paul Vixie
[NETFILTER] remove bogus hand-coded htonll() from nenetlink_queue
htonll() is nothing else than cpu_to_be64(), so we'd rather call the
latter.
Signed-off-by: Harald Welte <[EMAIL PROTECTED]>
---
commit 0905251a08bf51d5e2d1996c21fcdc5acfbbde13
tree a8072738e54f24b0d4392cf33252594d4a6848e1
parent e8d296c78dff8485c5cd90217b91433185a58871
author Harald Welte <[EMAIL PROTECTED]> Sa, 03 Sep 2005 10:31:19 +0200
committer Harald Welte <[EMAIL PROTECTED]> Sa, 03 Sep 2005 10:31:19 +0200
net/netfilter/nfnetlink_queue.c | 15 ++-
1 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -76,17 +76,6 @@ typedef int (*nfqnl_cmpfn)(struct nfqnl_
static DEFINE_RWLOCK(instances_lock);
-u_int64_t htonll(u_int64_t in)
-{
- u_int64_t out;
- int i;
-
- for (i = 0; i < sizeof(u_int64_t); i++)
- ((u_int8_t *))[sizeof(u_int64_t)-1] = ((u_int8_t *))[i];
-
- return out;
-}
-
#define INSTANCE_BUCKETS 16
static struct hlist_head instance_table[INSTANCE_BUCKETS];
@@ -497,8 +486,8 @@ nfqnl_build_packet_message(struct nfqnl_
if (entry->skb->tstamp.off_sec) {
struct nfqnl_msg_packet_timestamp ts;
- ts.sec = htonll(skb_tv_base.tv_sec +
entry->skb->tstamp.off_sec);
- ts.usec = htonll(skb_tv_base.tv_usec +
entry->skb->tstamp.off_usec);
+ ts.sec = cpu_to_be64(skb_tv_base.tv_sec +
entry->skb->tstamp.off_sec);
+ ts.usec = cpu_to_be64(skb_tv_base.tv_usec +
entry->skb->tstamp.off_usec);
NFA_PUT(skb, NFQA_TIMESTAMP, sizeof(ts), );
}
pgpfTvXdErwAO.pgp
Description: PGP signature
[PATCH 2/2] [NETFILTER] remove bogus hand-coded htonll()
Hi Dave, please apply the appended patch.
I somehow thought I had fixed this quite some time ago. Probably I lost
it with some merge :(
Thanks,
--
- Harald Welte [EMAIL PROTECTED] http://netfilter.org/
Fragmentation is like classful addressing -- an interesting early
architectural error that shows how much experimentation was going
on while IP was being designed.-- Paul Vixie
[NETFILTER] remove bogus hand-coded htonll() from nenetlink_queue
htonll() is nothing else than cpu_to_be64(), so we'd rather call the
latter.
Signed-off-by: Harald Welte [EMAIL PROTECTED]
---
commit 0905251a08bf51d5e2d1996c21fcdc5acfbbde13
tree a8072738e54f24b0d4392cf33252594d4a6848e1
parent e8d296c78dff8485c5cd90217b91433185a58871
author Harald Welte [EMAIL PROTECTED] Sa, 03 Sep 2005 10:31:19 +0200
committer Harald Welte [EMAIL PROTECTED] Sa, 03 Sep 2005 10:31:19 +0200
net/netfilter/nfnetlink_queue.c | 15 ++-
1 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -76,17 +76,6 @@ typedef int (*nfqnl_cmpfn)(struct nfqnl_
static DEFINE_RWLOCK(instances_lock);
-u_int64_t htonll(u_int64_t in)
-{
- u_int64_t out;
- int i;
-
- for (i = 0; i sizeof(u_int64_t); i++)
- ((u_int8_t *)out)[sizeof(u_int64_t)-1] = ((u_int8_t *)in)[i];
-
- return out;
-}
-
#define INSTANCE_BUCKETS 16
static struct hlist_head instance_table[INSTANCE_BUCKETS];
@@ -497,8 +486,8 @@ nfqnl_build_packet_message(struct nfqnl_
if (entry-skb-tstamp.off_sec) {
struct nfqnl_msg_packet_timestamp ts;
- ts.sec = htonll(skb_tv_base.tv_sec +
entry-skb-tstamp.off_sec);
- ts.usec = htonll(skb_tv_base.tv_usec +
entry-skb-tstamp.off_usec);
+ ts.sec = cpu_to_be64(skb_tv_base.tv_sec +
entry-skb-tstamp.off_sec);
+ ts.usec = cpu_to_be64(skb_tv_base.tv_usec +
entry-skb-tstamp.off_usec);
NFA_PUT(skb, NFQA_TIMESTAMP, sizeof(ts), ts);
}
pgpfTvXdErwAO.pgp
Description: PGP signature
Re: [PATCH 2/2] [NETFILTER] remove bogus hand-coded htonll()
On Sat, Sep 03, 2005 at 10:43:15AM +0200, Harald Welte wrote:
htonll() is nothing else than cpu_to_be64(), so we'd rather call the
latter.
Actually, the htonll() implementation does not seem to be doing what
cpu_to_be64() is doing.. However, I would assume this is a bug in
htonll() and this change to use cpu_to_be64() is fixing that. Can this
bug cause any major problems in the current implementation?
-u_int64_t htonll(u_int64_t in)
-{
- u_int64_t out;
- int i;
-
- for (i = 0; i sizeof(u_int64_t); i++)
- ((u_int8_t *)out)[sizeof(u_int64_t)-1] = ((u_int8_t *)in)[i];
I would assume that the first index should have had '-i' added to it, if
the purpose is to swap byte order.. The code here is leaving some
arbitrary data in 7 bytes of the 64-bit variable and setting
(u8*)out[7] = (u8*)in[7] in somewhat inefficient way ;-). In addition,
this looks more like swap-8-bytes-unconditionally than doing this based
on host byte order..
--
Jouni MalinenPGP id EFC895FA
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] [NETFILTER] remove bogus hand-coded htonll()
On Sat, Sep 03, 2005 at 09:54:25AM -0700, Jouni Malinen wrote: On Sat, Sep 03, 2005 at 10:43:15AM +0200, Harald Welte wrote: htonll() is nothing else than cpu_to_be64(), so we'd rather call the latter. Actually, the htonll() implementation does not seem to be doing what cpu_to_be64() is doing.. However, I would assume this is a bug in htonll() and this change to use cpu_to_be64() is fixing that. ACK. Can this bug cause any major problems in the current implementation? the current implementation was only merged after 2.6.13 is released, so I doubt anyone but the netfilter developers is using it yet. I would assume that the first index should have had '-i' added to it, if the purpose is to swap byte order.. The code here is leaving some arbitrary data in 7 bytes of the 64-bit variable and setting (u8*)out[7] = (u8*)in[7] in somewhat inefficient way ;-). In addition, this looks more like swap-8-bytes-unconditionally than doing this based on host byte order.. yes, yes, yes. Somehow this ancient buggy implementation slipped into mainline. I know I had fixed this before. So please let's all forget about this embarrassing htonll() and move on. -- - Harald Welte [EMAIL PROTECTED] http://netfilter.org/ Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed.-- Paul Vixie pgpZliKCIUmq8.pgp Description: PGP signature
