On Wednesday 24 June 2015 18:17:49 Ksenija Stanojevic wrote:

Replace all instances of time_t with time64_t i.e. change the type used for representing time from 32-bit to 64-bit. All 32-bit kernels to date use a signed 32-bit time_t type, which can only represent time until January 2038. The patch also changes the function get_seconds() that returns a 32-bit integer to ktime_get_seconds() that returns seconds as 64-bit integer and it uses monotonic instead of real time clock. Field expiry is changed to u64 in struct rxrpc_key_data_v1 and struct rxkad_key and to time64_t in key_preparsed_payload. This change is safe since field expiry is used only in this driver. Add macro ENCODE32 to export 64bit time to userspace as a 32bit data.

Signed-off-by: Ksenija Stanojevic ksenija.stanojevic@gmail.com

I think it would be better to split the patch into a small series. Changing all these types is by definition fragile, and I have a hard time following the logic.

I'd also suggest taking David Howells on Cc here, as he is the primary author of this code and he may have further thoughts.

I'll try to rearrange the patch a bit in my reply, in order to better comment on it.

diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h index fc48754..f13b2cb 100644 --- a/include/keys/rxrpc-type.h +++ b/include/keys/rxrpc-type.h @@ -27,7 +27,7 @@ extern struct key *rxrpc_get_null_key(const char *); struct rxkad_key { u32 vice_id; u32 start; /* time at which ticket starts */

• u32 expiry; /* time at which ticket expires */

• u64 expiry; /* time at which ticket expires */ u32 kvno; /* key version number */ u8 primary_flag; /* T if key for primary cell for this user */ u16 ticket_len; /* length of ticket[] */

So the expiry is currently an unsigned 32-bit value in this structure, presumably representing time from 1970 to 2106.

diff --git a/net/rxrpc/ar-key.c b/net/rxrpc/ar-key.c index db0f39f..c190cb1 100644 --- a/net/rxrpc/ar-key.c +++ b/net/rxrpc/ar-key.c @@ -125,14 +125,14 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep, token->kad->vice_id = ntohl(xdr[0]); token->kad->kvno = ntohl(xdr[1]); token->kad->start = ntohl(xdr[4]);

• token->kad->expiry = ntohl(xdr[5]);

• token->kad->expiry = (long long)ntohl(xdr[5]); token->kad->primary_flag = ntohl(xdr[6]); memcpy(&token->kad->session_key, &xdr[2], 8); memcpy(&token->kad->ticket, &xdr[8], tktlen);

However, here you convert it to a signed value, which keeps the same interpretation, but is a little confusing.

_debug("SCIX: %u", token->security_index); _debug("TLEN: %u", token->kad->ticket_len);

• _debug("EXPY: %x", token->kad->expiry);

• _debug("EXPY: %lld", token->kad->expiry); _debug("KVNO: %u", token->kad->kvno); _debug("PRIM: %u", token->kad->primary_flag); _debug("SKEY: %02x%02x%02x%02x%02x%02x%02x%02x",

@@ -727,7 +727,7 @@ static int rxrpc_preparse(struct key_preparsed_payload *prep) _debug("SCIX: %u", v1->security_index); _debug("TLEN: %u", v1->ticket_length);

• _debug("EXPY: %x", v1->expiry);

• _debug("EXPY: %lld", v1->expiry); _debug("KVNO: %u", v1->kvno); _debug("SKEY: %02x%02x%02x%02x%02x%02x%02x%02x", v1->session_key[0], v1->session_key[1],

Same here, you change from hexadecimal (unsigned by definition) to signed, but print an unsigned number. Again, the effect is the same, in particular because the range is known to be limited to an u32, but it's a bit confusing.

diff --git a/net/rxrpc/ar-connection.c b/net/rxrpc/ar-connection.c index 6631f4f..692b3e6 100644 --- a/net/rxrpc/ar-connection.c +++ b/net/rxrpc/ar-connection.c @@ -808,7 +808,7 @@ void rxrpc_put_connection(struct rxrpc_connection *conn) ASSERTCMP(atomic_read(&conn->usage), >, 0);

• conn->put_time = get_seconds();

• conn->put_time = ktime_get_seconds(); if (atomic_dec_and_test(&conn->usage)) { _debug("zombie"); rxrpc_queue_delayed_work(&rxrpc_connection_reap, 0);

@@ -852,7 +852,7 @@ static void rxrpc_connection_reaper(struct work_struct *work) _enter("");

• now = get_seconds();

• now = ktime_get_seconds(); earliest = ULONG_MAX;

write_lock_bh(&rxrpc_connection_lock); diff --git a/net/rxrpc/ar-transport.c b/net/rxrpc/ar-transport.c index 1976dec..9946467 100644 --- a/net/rxrpc/ar-transport.c +++ b/net/rxrpc/ar-transport.c @@ -189,7 +189,7 @@ void rxrpc_put_transport(struct rxrpc_transport *trans) ASSERTCMP(atomic_read(&trans->usage), >, 0);

• trans->put_time = get_seconds();

• trans->put_time = ktime_get_seconds(); if (unlikely(atomic_dec_and_test(&trans->usage))) { _debug("zombie"); /* let the reaper determine the timeout to avoid a race with

@@ -226,7 +226,7 @@ static void rxrpc_transport_reaper(struct work_struct *work) _enter("");

• now = get_seconds();

• now = ktime_get_seconds(); earliest = ULONG_MAX;

/* extract all the transports that have been dead too long */

The put_time changes look much simpler to me than the rest, and this can easily be split out from the rest of the changes into a separate self-contained patch, making it clear that those times are never transferred across the wire.

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index aef1bd2..24207db 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -208,7 +208,7 @@ struct rxrpc_transport { struct rb_root server_conns; /* server connections on this transport */ struct list_head link; /* link in master session list */ struct sk_buff_head error_queue; /* error packets awaiting processing */

• time_t put_time; /* time at which to reap */

• time64_t put_time; /* time at which to reap */ spinlock_t client_lock; /* client connection allocation lock */ rwlock_t conn_lock; /* lock for active/dead connections */ atomic_t usage;

@@ -256,7 +256,7 @@ struct rxrpc_connection { struct rxrpc_crypt csum_iv; /* packet checksum base */ unsigned long events; #define RXRPC_CONN_CHALLENGE 0 /* send challenge packet */

• time_t put_time; /* time at which to reap */

• time64_t put_time; /* time at which to reap */ rwlock_t lock; /* access lock */ spinlock_t state_lock; /* state-change lock */ atomic_t usage;

I'd use "unsigned long" as the type here, matching what you have in the local variables in rxrpc_connection_reaper, and change the comment to reflect that this is monotonic time.

We know that a 32-bit number is enough to store a monotonic time value (because we don't support systems don't get rebooted for 136 years), but we should also remove the 'time_t' getting mentioned here.

diff --git a/include/linux/key-type.h b/include/linux/key-type.h index ff9f1d3..ff59683 100644 --- a/include/linux/key-type.h +++ b/include/linux/key-type.h @@ -45,7 +45,7 @@ struct key_preparsed_payload { const void *data; /* Raw data */ size_t datalen; /* Raw datalen */ size_t quotalen; /* Quota length for proposed payload */

• time_t expiry; /* Expiry time of key */

• time64_t expiry; /* Expiry time of key */ bool trusted; /* True if key is trusted */

};

This member is accessed in security/keys/key.c:__key_instantiate_and_link():

if (prep->expiry != TIME_T_MAX) { key->expiry = prep->expiry; key_schedule_gc(prep->expiry + key_gc_delay); }

When we change the type here, we also have to update the code in that file, especially as TIME_T_MAX can now be a valid timeout in 2038.

The keys/key.c code sets TIME_T_MAX as the default, and out of the many implementations in the kernel, only the rxrpc one overrides this, so we should be able to change it to TIME64_T_MAX, but there should be one patch that takes care of the key_preparsed_payload in both ends of it, and that patch should explain how you have determined that all uses of this field are safe to be changed in this manner.

@@ -101,7 +101,7 @@ struct rxrpc_key_token { struct rxrpc_key_data_v1 { u16 security_index; u16 ticket_length;

• u32 expiry; /* time_t */

• u64 expiry; /* time_t */ u32 kvno; u8 session_key[8]; u8 ticket[0];

The structure matches the binary layout described in the comment for the rxrpc_preparse() function, I don't think it's safe to change it here, as we assume that we can simply interpret the data passed into prep->data using this format.

I can see two options here:

a) redefine the type to explicitly be expressed as 'unsigned 32-bit seconds' rather than 'time_t' that is signed. This would let us keep using rxrpc_key_data_v1 until 2106, or add another hack on top then. If we do this, there should be an explicit way to convert back and forth between the 'u32 expiry' and a 'time64_t' with some helper function, ideally one that warns in case of an overflow.

b) define a new rxrpc_key_data_v2 and make the code handle both versions, so we always store keys as v2 but are able to read both v1 and v2 keys.

@@ -1134,6 +1134,12 @@ static long rxrpc_read(const struct key *key, if (put_user(y, xdr++) < 0) \ goto fault; \ } while(0) +#define ENCODE32(x) \

• do { \

• u32 z = (u32) x;		\
  

• ENCODE(z);			\
  

• } while(0)

#define ENCODE_DATA(l, s) \ do { \ u32 _l = (l); \ @@ -1175,7 +1181,7 @@ static long rxrpc_read(const struct key *key, ENCODE(token->kad->kvno); ENCODE_DATA(8, token->kad->session_key); ENCODE(token->kad->start);

• 	ENCODE(token->kad->expiry);
  

• 	ENCODE32(token->kad->expiry);
  ENCODE(token->kad->primary_flag);
  ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
  break;
  

Here you have a silent truncation of the expiry value when copying to user space. You have mentioned in the changelog that you now do this, but what we really need is either an marker in the code that lets us get back to this later if it's broken, or an explanation about why we have concluded that it is safe, and within which constraints that is true (e.g. defining this to be valid until 2106).

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c index f226709..7c42437 100644 --- a/net/rxrpc/rxkad.c +++ b/net/rxrpc/rxkad.c @@ -819,7 +820,7 @@ protocol_error: static int rxkad_decrypt_ticket(struct rxrpc_connection *conn, void *ticket, size_t ticket_len, struct rxrpc_crypt *_session_key,

• 		time_t *_expiry,
  

• 		time64_t *_expiry,
  	u32 *_abort_code)
  

{ struct blkcipher_desc desc; @@ -827,7 +828,7 @@ static int rxkad_decrypt_ticket(struct rxrpc_connection *conn, struct scatterlist sg[1]; struct in_addr addr; unsigned int life;

• time_t issue, now;

• time64_t issue, now; bool little_endian; int ret; u8 *p, *q, *name, *end;

@@ -915,15 +916,15 @@ static int rxkad_decrypt_ticket(struct rxrpc_connection *conn, if (little_endian) { __le32 stamp; memcpy(&stamp, p, 4);

• issue = le32_to_cpu(stamp);
  

• issue = le64_to_cpu((__le64)stamp);
  

  } else { __be32 stamp; memcpy(&stamp, p, 4);

• issue = be32_to_cpu(stamp);
  

• issue = be64_to_cpu((__be64)stamp);
  

  }

This is an incorrect endian conversion: be64_to_cpu((__be64)stamp) will put the seconds in the upper half of 'issue' variable. The 32-bit time stamp in the source data is defined as the 'kerberos IV ticket', and we cannot make that 64-bit.

This means that here we have to change the interpretation of the 32-bit value. This is separate from the rxrpc_key_data_v1 structure, but we can do the same approach I described above and add a helper function that turns the 32-bit value into a time64_t, such as this:

static time64_t rxkad_parse_timestamp(void *p, bool little_endian) { u32 time32;

if (little_endian) { __le32 stamp; memcpy(&stamp, p, 4); time32 = le32_to_cpu(stamp); } else { __be32 stamp; memcpy(&stamp, p, 4); time32 = be32_to_cpu(stamp); }

/* * The kerberos 4 time value is traditionally a 32-bit time_t, which * overflows in 2038. By interpreting it as an unsigned value here, we * gain an extra 68 years until 2106. This works because we know the * timestamp can never be negative. */

return (u64)time32; }

p += 4;

• now = get_seconds();
• _debug("KIV ISSUE: %lx [%lx]", issue, now);

• now = ktime_get_seconds();
• _debug("KIV ISSUE: %lld [%lld]", issue, now);

/* check the ticket is in date */ if (issue > now) {

Here, we have an incorrect use of ktime_get_seconds(): the 'issue' time stamp we get here comes from outside, and if we want to compare that to the current time, we cannot use the monotonic time, but have to use ktime_get_real_seconds() instead.

Arnd

Hi,

I have a question concerning put_time fields.

...

diff --git a/net/rxrpc/ar-connection.c b/net/rxrpc/ar-connection.c index 6631f4f..692b3e6 100644 --- a/net/rxrpc/ar-connection.c +++ b/net/rxrpc/ar-connection.c @@ -808,7 +808,7 @@ void rxrpc_put_connection(struct rxrpc_connection *conn)

  ASSERTCMP(atomic_read(&conn->usage), >, 0);

• conn->put_time = get_seconds();
  

• conn->put_time = ktime_get_seconds();
  if (atomic_dec_and_test(&conn->usage)) {
          _debug("zombie");
          rxrpc_queue_delayed_work(&rxrpc_connection_reap, 0);
  

@@ -852,7 +852,7 @@ static void rxrpc_connection_reaper(struct work_struct *work)

  _enter("");

• now = get_seconds();
  

• now = ktime_get_seconds();
  earliest = ULONG_MAX;
  
  write_lock_bh(&rxrpc_connection_lock);
  

diff --git a/net/rxrpc/ar-transport.c b/net/rxrpc/ar-transport.c index 1976dec..9946467 100644 --- a/net/rxrpc/ar-transport.c +++ b/net/rxrpc/ar-transport.c @@ -189,7 +189,7 @@ void rxrpc_put_transport(struct rxrpc_transport *trans)

  ASSERTCMP(atomic_read(&trans->usage), >, 0);

• trans->put_time = get_seconds();
  

• trans->put_time = ktime_get_seconds();
  

ktime_get_seconds returns time64_t

...

  if (unlikely(atomic_dec_and_test(&trans->usage))) {
          _debug("zombie");
          /* let the reaper determine the timeout to avoid a race with

@@ -226,7 +226,7 @@ static void rxrpc_transport_reaper(struct work_struct *work)

  _enter("");

• now = get_seconds();
  

• now = ktime_get_seconds();
  earliest = ULONG_MAX;
  
  /* extract all the transports that have been dead too long */
  

The put_time changes look much simpler to me than the rest, and this can easily be split out from the rest of the changes into a separate self-contained patch, making it clear that those times are never transferred across the wire.

...

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index aef1bd2..24207db 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -208,7 +208,7 @@ struct rxrpc_transport { struct rb_root server_conns; /* server connections on this transport */ struct list_head link; /* link in master session list */ struct sk_buff_head error_queue; /* error packets awaiting processing */

• time_t                  put_time;       /* time at which to reap */
  

• time64_t                        put_time;       /* time at which to reap */
  spinlock_t              client_lock;    /* client connection allocation lock */
  rwlock_t                conn_lock;      /* lock for active/dead connections */
  atomic_t                usage;
  

@@ -256,7 +256,7 @@ struct rxrpc_connection { struct rxrpc_crypt csum_iv; /* packet checksum base */ unsigned long events; #define RXRPC_CONN_CHALLENGE 0 /* send challenge packet */

• time_t                  put_time;       /* time at which to reap */
  

• time64_t                        put_time;       /* time at which to reap */
  rwlock_t                lock;           /* access lock */
  spinlock_t              state_lock;     /* state-change lock */
  atomic_t                usage;
  

I'd use "unsigned long" as the type here, matching what you have in the local variables in rxrpc_connection_reaper, and change the comment to reflect that this is monotonic time.

If i use unsigned long instead time64_t then I cannot use ktime_get_seconds? Should I then leave get_seconds or use some other function?

We know that a 32-bit number is enough to store a monotonic time value (because we don't support systems don't get rebooted for 136 years), but we should also remove the 'time_t' getting mentioned here.

Thanks,

Ksenija