Re: [PATCH 2/3] Make sockets proper objhash objects and use checkpoint_obj() on them

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Dan Smith wrote:
> OL> It's perhaps more accurate to s/most sockets/some sockets/. It may
> OL> be more likely for a socket to be checkpointed as a peer of
> OL> another process, or as the sender of an skb.
> 
> Um, how about "most of the time" ?  I definitely think that the
> (overwhelmingly) common case is a pair of sockets each attached to a
> file descriptor.
> 
> OL> Now that you made 'struct sock' a 1st class object, they deserve to
> OL> enjoy 1st class treatment :p  That also means proper collect() method
> OL> - probably starting with the f_op ...
> 
> Okay.
> 
> OL> I may be mistaken, but I suspect that the suggested implementation
> OL> cannot limit the depth of recursive calls to checkpoint_obj(). For
> OL> instance, consider a dgram socket that received data from another
> OL> dgram socket, that received data from another dgram, ad infinitum.
> 
> At the very least, a single receive socket is limited in how many
> skb's may be queued for it, which limits an attacker's ability to
> reach the "ad infinitum" case, I'd say.  Do we need something more?

Multiple buffers adds iteration, and one level of recursion. I had in
mind a slightly different scenario: instead of many buffers for one
socket, many sockets "chained" -

Assume N sockets S_1...S_n, all dgram, none is connected. Each socket
S_i send one packet to S_i+1.  Suppose you first checkpoint S_n, then
you'll need to checkpoint S_n-1, for which you'll need to checkpoint
S_n-2 etc.

> OL> I'm thinking about the two other use cases that I mentioned:
> OL> "dangling" (not-referenced by a file) and "pending" (not yet
> OL> accepted) sockets.
> 
> OL> In both cases (well, at least with "pending"), the 'struct sock'
> OL> exist but the 'struct socket' does not exit until after the socket
> OL> is attached to a file descriptor. IIRC, the lifespan of 'struct
> OL> socket' is coupled to that of the referencing file.
> 
> OL> In that case, I guess it make more sense to leave the 'struct
> OL> socket' related data within ckpt_hdr_file_socket.
> 
> Hmm, not by my reading.  From what I can tell, the accept operation

You are right: sock_init_data() sets it up, and I believe it is
for the entire lifetime of the sock/socket.

>>> +		return ERR_PTR(PTR_ERR(sk));
> 
> OL> Nit: I vaguely recall some disapproval of such construct...
> OL> How about '(struct file *) sk' ?
> 
> Casting it to the wrong type seems less desirable to me.  I was
> following the lead of:
> 
>   % fgrep -r 'ERR_PTR(PTR_ERR' . | wc -l
>   36

Yep. That settles it then :)

> 
> and:
> 
>   % fgrep -r 'ERR_PTR(PTR_ERR' checkpoint
>   checkpoint/namespace.c:		return ERR_PTR(PTR_ERR(h));
>   checkpoint/signal.c:		return ERR_PTR(PTR_ERR(h));

(FWIW, this was criticized ...)

Oren.