Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] hcidump: add support for time64 based libc
@ 2020-01-10 20:49 Arnd Bergmann
  2020-01-10 21:05 ` Rich Felker
  2020-01-10 23:30 ` Guy Harris
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2020-01-10 20:49 UTC (permalink / raw)
  To: Bluez mailing list
  Cc: y2038 Mailman List, Johan Hedberg, Linux Kernel Mailing List,
	Deepa Dinamani, Rich Felker, Guy Harris, Arnd Bergmann

musl is moving to a default of 64-bit time_t on all architectures,
glibc will follow later. This breaks reading timestamps through cmsg
data with the HCI_TIME_STAMP socket option.

Change both copies of hcidump to work on all architectures.  This also
fixes x32, which has never worked, and carefully avoids breaking sparc64,
which is another special case.

I have only compiled this on one architecture, please at least test
it to ensure there are no regressions. The toolchain binaries from
http://musl.cc/ should allow testing with a 64-bit time_t, but it may
be hard to build all the dependencies first.

libpcap has the same bug and needs a similar fix to work on future
32-bit Linux systems. Everything else apparently uses the generic
SO_TIMESTAMP timestamps, which work correctly when using new enough
kernels with a time64 libc.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 monitor/hcidump.c | 32 +++++++++++++++++++++++++++++++-
 tools/hcidump.c   | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/monitor/hcidump.c b/monitor/hcidump.c
index 8b6f846d3..6d2330287 100644
--- a/monitor/hcidump.c
+++ b/monitor/hcidump.c
@@ -107,6 +107,36 @@ static int open_hci_dev(uint16_t index)
 	return fd;
 }
 
+static struct timeval hci_tstamp_read(void *data)
+{
+	struct timeval tv;
+
+	/*
+	 * On 64-bit architectures, the data matches the timeval
+	 * format. Note that on sparc64 this is different from
+	 * all others.
+	 */
+	if (sizeof(long) == 8) {
+		memcpy(&tv, data, sizeof(tv));
+	}
+
+	/*
+	 * On 32-bit architectures, the timeval definition may
+	 * use 32-bit or 64-bit members depending on the C
+	 * library and architecture.
+	 * The cmsg data however always contains a pair of
+	 * 32-bit values. Interpret as unsigned to make it work
+	 * past y2038.
+	 */
+	if (sizeof(long) == 4) {
+		unsigned int *stamp = data;
+		tv.tv_sec = stamp[0];
+		tv.tv_usec = stamp[1];
+	}
+
+	return tv;
+}
+
 static void device_callback(int fd, uint32_t events, void *user_data)
 {
 	struct hcidump_data *data = user_data;
@@ -150,7 +180,7 @@ static void device_callback(int fd, uint32_t events, void *user_data)
 				memcpy(&dir, CMSG_DATA(cmsg), sizeof(dir));
 				break;
 			case HCI_CMSG_TSTAMP:
-				memcpy(&ctv, CMSG_DATA(cmsg), sizeof(ctv));
+				ctv = hci_tstamp_read(CMSG_DATA(cmsg));
 				tv = &ctv;
 				break;
 			}
diff --git a/tools/hcidump.c b/tools/hcidump.c
index 33d429b6c..be14d0930 100644
--- a/tools/hcidump.c
+++ b/tools/hcidump.c
@@ -136,6 +136,36 @@ static inline int write_n(int fd, char *buf, int len)
 	return t;
 }
 
+static struct timeval hci_tstamp_read(void *data)
+{
+	struct timeval tv;
+
+	/*
+	 * On 64-bit architectures, the data matches the timeval
+	 * format. Note that on sparc64 this is different from
+	 * all others.
+	 */
+	if (sizeof(long) == 8) {
+		memcpy(&tv, data, sizeof(tv));
+	}
+
+	/*
+	 * On 32-bit architectures, the timeval definition may
+	 * use 32-bit or 64-bit members depending on the C
+	 * library and architecture.
+	 * The cmsg data however always contains a pair of
+	 * 32-bit values. Interpret as unsigned to make it work
+	 * past y2038.
+	 */
+	if (sizeof(long) == 4) {
+		unsigned int *stamp = data;
+		tv.tv_sec = stamp[0];
+		tv.tv_usec = stamp[1];
+	}
+
+	return tv;
+}
+
 static int process_frames(int dev, int sock, int fd, unsigned long flags)
 {
 	struct cmsghdr *cmsg;
@@ -230,8 +260,7 @@ static int process_frames(int dev, int sock, int fd, unsigned long flags)
 				frm.in = (uint8_t) dir;
 				break;
 			case HCI_CMSG_TSTAMP:
-				memcpy(&frm.ts, CMSG_DATA(cmsg),
-						sizeof(struct timeval));
+				frm.ts = hci_tstamp_read(CMSG_DATA(cmsg));
 				break;
 			}
 			cmsg = CMSG_NXTHDR(&msg, cmsg);
-- 
2.20.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hcidump: add support for time64 based libc
  2020-01-10 20:49 [PATCH] hcidump: add support for time64 based libc Arnd Bergmann
@ 2020-01-10 21:05 ` Rich Felker
  2020-01-10 21:19   ` Arnd Bergmann
  2020-01-10 23:30 ` Guy Harris
  1 sibling, 1 reply; 6+ messages in thread
From: Rich Felker @ 2020-01-10 21:05 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bluez mailing list, y2038 Mailman List, Johan Hedberg,
	Linux Kernel Mailing List, Deepa Dinamani, Guy Harris

On Fri, Jan 10, 2020 at 09:49:03PM +0100, Arnd Bergmann wrote:
> musl is moving to a default of 64-bit time_t on all architectures,
> glibc will follow later. This breaks reading timestamps through cmsg
> data with the HCI_TIME_STAMP socket option.
> 
> Change both copies of hcidump to work on all architectures.  This also
> fixes x32, which has never worked, and carefully avoids breaking sparc64,
> which is another special case.

Won't it be broken on rv32 though? Based on my (albeit perhaps
incomplete) reading of the thread, I think use of HCI_TIME_STAMP
should just be dropped entirely in favor of using SO_TIMESTAMPNS -- my
understanding was that it works with bluetooth sockets too.

Rich

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hcidump: add support for time64 based libc
  2020-01-10 21:05 ` Rich Felker
@ 2020-01-10 21:19   ` Arnd Bergmann
  2020-01-10 23:32     ` Guy Harris
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2020-01-10 21:19 UTC (permalink / raw)
  To: Rich Felker
  Cc: Bluez mailing list, y2038 Mailman List, Johan Hedberg,
	Linux Kernel Mailing List, Deepa Dinamani, Guy Harris

On Fri, Jan 10, 2020 at 10:05 PM Rich Felker <dalias@libc.org> wrote:
>
> On Fri, Jan 10, 2020 at 09:49:03PM +0100, Arnd Bergmann wrote:
> > musl is moving to a default of 64-bit time_t on all architectures,
> > glibc will follow later. This breaks reading timestamps through cmsg
> > data with the HCI_TIME_STAMP socket option.
> >
> > Change both copies of hcidump to work on all architectures.  This also
> > fixes x32, which has never worked, and carefully avoids breaking sparc64,
> > which is another special case.
>
> Won't it be broken on rv32 though? Based on my (albeit perhaps
> incomplete) reading of the thread, I think use of HCI_TIME_STAMP
> should just be dropped entirely in favor of using SO_TIMESTAMPNS -- my
> understanding was that it works with bluetooth sockets too.

All 32-bit architectures use old_timeval32 timestamps in the kernel
here, even rv32 and x32. As a rule, we keep the types bug-for-bug
compatible between architectures and fix them all at the same time.

Changing hcidump to SO_TIMESTAMPNS would work as well, but
that is a much bigger change and I don't know how to test that.

     Arnd

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hcidump: add support for time64 based libc
  2020-01-10 20:49 [PATCH] hcidump: add support for time64 based libc Arnd Bergmann
  2020-01-10 21:05 ` Rich Felker
@ 2020-01-10 23:30 ` Guy Harris
  1 sibling, 0 replies; 6+ messages in thread
From: Guy Harris @ 2020-01-10 23:30 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bluez mailing list, y2038 Mailman List, Johan Hedberg,
	Linux Kernel Mailing List, Deepa Dinamani, Rich Felker

On Jan 10, 2020, at 12:49 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> diff --git a/monitor/hcidump.c b/monitor/hcidump.c
> index 8b6f846d3..6d2330287 100644
> --- a/monitor/hcidump.c
> +++ b/monitor/hcidump.c
> @@ -107,6 +107,36 @@ static int open_hci_dev(uint16_t index)
> 	return fd;
> }
> 
> +static struct timeval hci_tstamp_read(void *data)
> +{
> +	struct timeval tv;
> +
> +	/*
> +	 * On 64-bit architectures, the data matches the timeval
> +	 * format. Note that on sparc64 this is different from
> +	 * all others.
> +	 */
> +	if (sizeof(long) == 8) {
> +		memcpy(&tv, data, sizeof(tv));
> +	}
> +
> +	/*
> +	 * On 32-bit architectures, the timeval definition may
> +	 * use 32-bit or 64-bit members depending on the C
> +	 * library and architecture.
> +	 * The cmsg data however always contains a pair of
> +	 * 32-bit values. Interpret as unsigned to make it work
> +	 * past y2038.
> +	 */
> +	if (sizeof(long) == 4) {
> +		unsigned int *stamp = data;
> +		tv.tv_sec = stamp[0];
> +		tv.tv_usec = stamp[1];
> +	}
> +
> +	return tv;
> +}

Should it be something more like

	if (sizeof(long) == 8) {
		/*
		 * On 64-bit architectures, the data matches the timeval
		 * format. Note that on sparc64 this is different from
		 * all others.
		 */
		memcpy(&tv, data, sizeof(tv));
	} else if (sizeof(long) == 4) {
		/*
		 * On 32-bit architectures, the timeval definition may
		 * use 32-bit or 64-bit members depending on the C
		 * library and architecture.
		 * The cmsg data however always contains a pair of
		 * 32-bit values. Interpret as unsigned to make it work
		 * past y2038.
		 */
		unsigned int *stamp = data;
		tv.tv_sec = stamp[0];
		tv.tv_usec = stamp[1];
	} else {
		abort();	/* or some other "sorry, we're not ready for 128-bit or weird architectures yet" failure */
	}

	return tv;

> static void device_callback(int fd, uint32_t events, void *user_data)
> {
> 	struct hcidump_data *data = user_data;
> @@ -150,7 +180,7 @@ static void device_callback(int fd, uint32_t events, void *user_data)
> 				memcpy(&dir, CMSG_DATA(cmsg), sizeof(dir));
> 				break;
> 			case HCI_CMSG_TSTAMP:
> -				memcpy(&ctv, CMSG_DATA(cmsg), sizeof(ctv));
> +				ctv = hci_tstamp_read(CMSG_DATA(cmsg));
> 				tv = &ctv;
> 				break;
> 			}

And libpcap's Linux BT code should do the same thing, changing its memcpy() call?

If you want, you can submit a pull request, or I can make the change.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hcidump: add support for time64 based libc
  2020-01-10 21:19   ` Arnd Bergmann
@ 2020-01-10 23:32     ` Guy Harris
  2020-01-11 16:21       ` Arnd Bergmann
  0 siblings, 1 reply; 6+ messages in thread
From: Guy Harris @ 2020-01-10 23:32 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rich Felker, Bluez mailing list, y2038 Mailman List,
	Johan Hedberg, Linux Kernel Mailing List, Deepa Dinamani

On Jan 10, 2020, at 1:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> On Fri, Jan 10, 2020 at 10:05 PM Rich Felker <dalias@libc.org> wrote:
>> 
>> On Fri, Jan 10, 2020 at 09:49:03PM +0100, Arnd Bergmann wrote:
>>> musl is moving to a default of 64-bit time_t on all architectures,
>>> glibc will follow later. This breaks reading timestamps through cmsg
>>> data with the HCI_TIME_STAMP socket option.
>>> 
>>> Change both copies of hcidump to work on all architectures.  This also
>>> fixes x32, which has never worked, and carefully avoids breaking sparc64,
>>> which is another special case.
>> 
>> Won't it be broken on rv32 though? Based on my (albeit perhaps
>> incomplete) reading of the thread, I think use of HCI_TIME_STAMP
>> should just be dropped entirely in favor of using SO_TIMESTAMPNS -- my
>> understanding was that it works with bluetooth sockets too.
> 
> All 32-bit architectures use old_timeval32 timestamps in the kernel
> here, even rv32 and x32. As a rule, we keep the types bug-for-bug
> compatible between architectures and fix them all at the same time.
> 
> Changing hcidump to SO_TIMESTAMPNS would work as well, but
> that is a much bigger change and I don't know how to test that.

If so, maybe I'll just do that for libpcap.  Libpcap *does* have an API to request capturing with nanoseconds in tv_usec (and I plan to give it pcapng-flavored APIs to deliver higher-resolution time stamps, as well as metadata such as "incoming" vs. "outgoing", as well).

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hcidump: add support for time64 based libc
  2020-01-10 23:32     ` Guy Harris
@ 2020-01-11 16:21       ` Arnd Bergmann
  0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2020-01-11 16:21 UTC (permalink / raw)
  To: Guy Harris
  Cc: Rich Felker, Bluez mailing list, y2038 Mailman List,
	Johan Hedberg, Linux Kernel Mailing List, Deepa Dinamani

 was On Sat, Jan 11, 2020 at 12:32 AM Guy Harris <guy@alum.mit.edu> wrote:
>
> On Jan 10, 2020, at 1:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> > On Fri, Jan 10, 2020 at 10:05 PM Rich Felker <dalias@libc.org> wrote:
> >>
> >> On Fri, Jan 10, 2020 at 09:49:03PM +0100, Arnd Bergmann wrote:
> >>> musl is moving to a default of 64-bit time_t on all architectures,
> >>> glibc will follow later. This breaks reading timestamps through cmsg
> >>> data with the HCI_TIME_STAMP socket option.
> >>>
> >>> Change both copies of hcidump to work on all architectures.  This also
> >>> fixes x32, which has never worked, and carefully avoids breaking sparc64,
> >>> which is another special case.
> >>
> >> Won't it be broken on rv32 though? Based on my (albeit perhaps
> >> incomplete) reading of the thread, I think use of HCI_TIME_STAMP
> >> should just be dropped entirely in favor of using SO_TIMESTAMPNS -- my
> >> understanding was that it works with bluetooth sockets too.
> >
> > All 32-bit architectures use old_timeval32 timestamps in the kernel
> > here, even rv32 and x32. As a rule, we keep the types bug-for-bug
> > compatible between architectures and fix them all at the same time.
> >
> > Changing hcidump to SO_TIMESTAMPNS would work as well, but
> > that is a much bigger change and I don't know how to test that.
>
> If so, maybe I'll just do that for libpcap.  Libpcap *does* have an API to request
>capturing with nanoseconds in tv_usec (and I plan to give it pcapng-flavored APIs
> to deliver higher-resolution time stamps, as well as metadata such as "incoming"
> vs. "outgoing", as well).

Sounds good to me, just make sure that you no longer reference
HCI_TIME_STAMP and it should be fine with both old and new libc
versions.

SO_TIMESTAMPNS was first added to the kernel in linux-2.6.22, which
is probably older than anything you care about,  but I'm not sure what you
need to do to use it on bluetooth. It appears to have first been added along
with the monitor channel support in linux-3.4.

On a semi-related note, I see that there is a y2038 compatibility in the
libpcap definition of struct pcap_timeval, which uses a *signed* tv_sec
field. I assume the sizes of the fields cannot be changed, but it would
be good to investigate if you can just make it unsigned instead, changing
the supported time range from 1902...2038 to 1970...2106.
It probably makes sense to do this together with supporting nanosecond
timestamps. You could also consider changing the format to use a
64-bit nanosecond value starting at either 1970 or 1902 to give an
even larger range.

       Arnd

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10 20:49 [PATCH] hcidump: add support for time64 based libc Arnd Bergmann
2020-01-10 21:05 ` Rich Felker
2020-01-10 21:19   ` Arnd Bergmann
2020-01-10 23:32     ` Guy Harris
2020-01-11 16:21       ` Arnd Bergmann
2020-01-10 23:30 ` Guy Harris

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git